I2P_Developers/libsam3
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Buffer overflow detected #2

New Issue
Closed
opened 2025-04-21 14:42:46 -04:00 by idk · 5 comments
idk commented 2025-04-21 14:42:46 -04:00
Owner
Copy Link

Hello! Please excuse my ignorance, I am just getting started with i2p and the SAM library. When I build and run the libsam3 stream examples "streams.c" and "streamss.c" I get "buffer overflow detected" error, which terminate the program.

I am running Linux, and have my i2prouter running with the SAM client running.

Hello! Please excuse my ignorance, I am just getting started with i2p and the SAM library. When I build and run the libsam3 stream examples "streams.c" and "streamss.c" I get "buffer overflow detected" error, which terminate the program. I am running Linux, and have my i2prouter running with the SAM client running.
idk self-assigned this 2025-04-21 14:42:46 -04:00
idk closed this issue 2025-04-21 14:42:46 -04:00
idk commented 2025-04-21 14:42:46 -04:00
Author
Owner
Copy Link

Thanks very much @obscuratus for bumping this back into my view and for providing a fix, I'll review, test, and in all likelihood merge this patch shortly.

Thanks very much @obscuratus for bumping this back into my view and for providing a fix, I'll review, test, and in all likelihood merge this patch shortly.
idk commented 2025-04-21 14:42:47 -04:00
Author
Owner
Copy Link

Here's my proposed fix:

From: obscuratus <obscuratus@mail.i2p>
Date: Sun, 31 Jul 2022 13:23:08 +0000
Subject: [PATCH] Fix Buffer Overflow resulting from varying key sizes.

Signed-off-by: obscuratus <obscuratus@mail.i2p>
---
 src/libsam3/libsam3.c | 4 ++++
 src/libsam3/libsam3.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/libsam3/libsam3.c b/src/libsam3/libsam3.c
index c596bb9..2c46ab3 100644
--- a/src/libsam3/libsam3.c
+++ b/src/libsam3/libsam3.c
@@ -896,6 +896,10 @@ int sam3CreateSession(Sam3Session *ses, const char *hostname, int port,
       goto error;
     }
     // save our keys
+    if (strlen(v) > SAM3_PRIVKEY_MAX_SIZE) {
+        fprintf(stderr, "ERROR, Unexpected key size (%li)!\n", strlen(v));
+        goto error;
+    }
     strcpy(ses->privkey, v);
     sam3FreeFieldList(rep);
     // get public key
diff --git a/src/libsam3/libsam3.h b/src/libsam3/libsam3.h
index 16942f5..16239bf 100644
--- a/src/libsam3/libsam3.h
+++ b/src/libsam3/libsam3.h
@@ -39,6 +39,7 @@ extern int libsam3_debug;
 #define SAM3_PUBKEY_SIZE (516)
 #define SAM3_CERT_SIZE (100)
 #define SAM3_PRIVKEY_MIN_SIZE (884)
+#define SAM3_PRIVKEY_MAX_SIZE (1024)
 
 ////////////////////////////////////////////////////////////////////////////////
 /* returns fd or -1 */
@@ -136,7 +137,7 @@ typedef struct Sam3Session {
   Sam3SessionType type;
   Sam3SigType sigType;
   int fd;
-  char privkey[SAM3_PRIVKEY_MIN_SIZE + 1]; // destination private key (asciiz)
+  char privkey[SAM3_PRIVKEY_MAX_SIZE + 1]; // destination private key (asciiz)
   char pubkey[SAM3_PUBKEY_SIZE + SAM3_CERT_SIZE +
               1];   // destination public key (asciiz)
   char channel[66]; // name of this sam session (asciiz)
-- 
2.35.1
Here's my proposed fix: ``` From: obscuratus <obscuratus@mail.i2p> Date: Sun, 31 Jul 2022 13:23:08 +0000 Subject: [PATCH] Fix Buffer Overflow resulting from varying key sizes. Signed-off-by: obscuratus <obscuratus@mail.i2p> --- src/libsam3/libsam3.c | 4 ++++ src/libsam3/libsam3.h | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libsam3/libsam3.c b/src/libsam3/libsam3.c index c596bb9..2c46ab3 100644 --- a/src/libsam3/libsam3.c +++ b/src/libsam3/libsam3.c @@ -896,6 +896,10 @@ int sam3CreateSession(Sam3Session *ses, const char *hostname, int port, goto error; } // save our keys + if (strlen(v) > SAM3_PRIVKEY_MAX_SIZE) { + fprintf(stderr, "ERROR, Unexpected key size (%li)!\n", strlen(v)); + goto error; + } strcpy(ses->privkey, v); sam3FreeFieldList(rep); // get public key diff --git a/src/libsam3/libsam3.h b/src/libsam3/libsam3.h index 16942f5..16239bf 100644 --- a/src/libsam3/libsam3.h +++ b/src/libsam3/libsam3.h @@ -39,6 +39,7 @@ extern int libsam3_debug; #define SAM3_PUBKEY_SIZE (516) #define SAM3_CERT_SIZE (100) #define SAM3_PRIVKEY_MIN_SIZE (884) +#define SAM3_PRIVKEY_MAX_SIZE (1024) //////////////////////////////////////////////////////////////////////////////// /* returns fd or -1 */ @@ -136,7 +137,7 @@ typedef struct Sam3Session { Sam3SessionType type; Sam3SigType sigType; int fd; - char privkey[SAM3_PRIVKEY_MIN_SIZE + 1]; // destination private key (asciiz) + char privkey[SAM3_PRIVKEY_MAX_SIZE + 1]; // destination private key (asciiz) char pubkey[SAM3_PUBKEY_SIZE + SAM3_CERT_SIZE + 1]; // destination public key (asciiz) char channel[66]; // name of this sam session (asciiz) -- 2.35.1 ```
idk commented 2025-04-21 14:42:47 -04:00
Author
Owner
Copy Link

@idk :
I think I've found the problem.

Looking at this strcpy at line 899 of libsam3.c

    // save our keys
    strcpy(ses->privkey, v);
    sam3FreeFieldList(rep);

I've been getting a buffer overflow here.

I added some debugging comments, and sure enough, the strlen of v was 908

privkey is defined in the Sam3Session structure as:

  char privkey[SAM3_PRIVKEY_MIN_SIZE + 1]; // destination private key (asciiz)

where SAM3_PRIVKEY_MIN_SIZE is

#define SAM3_PRIVKEY_MIN_SIZE (884)

If I change SAM3_PRIVKEY_MIN_SIZE to 908, the program runs.

But it worries me that it seems necessary to anticipate the key size exactly.

If I change SAM3_PRIVKEY_MIN_SIZE to 1024, libsam3 kicks this key out for being too small.

So changing the SAM3_PRIVKEY_MIN_SIZE is a work-around, but it leaves libsam3 not very robust with respect to keys of varying size, I think.

@idk : I think I've found the problem. Looking at this strcpy at line 899 of libsam3.c ``` // save our keys strcpy(ses->privkey, v); sam3FreeFieldList(rep); ``` I've been getting a buffer overflow here. I added some debugging comments, and sure enough, the strlen of v was 908 privkey is defined in the Sam3Session structure as: ``` char privkey[SAM3_PRIVKEY_MIN_SIZE + 1]; // destination private key (asciiz) ``` where SAM3_PRIVKEY_MIN_SIZE is ``` #define SAM3_PRIVKEY_MIN_SIZE (884) ``` If I change SAM3_PRIVKEY_MIN_SIZE to 908, the program runs. But it worries me that it seems necessary to anticipate the key size exactly. If I change SAM3_PRIVKEY_MIN_SIZE to 1024, libsam3 kicks this key out for being too small. So changing the SAM3_PRIVKEY_MIN_SIZE is a work-around, but it leaves libsam3 not very robust with respect to keys of varying size, I think.
idk commented 2025-04-21 14:42:47 -04:00
Author
Owner
Copy Link

Thank you! I'm trying to debug the problem too, I'll make a comment in this thread or a new MR if I figure anything out.

Thank you! I'm trying to debug the problem too, I'll make a comment in this thread or a new MR if I figure anything out.
idk commented 2025-04-21 14:42:47 -04:00
Author
Owner
Copy Link

Thanks for the report b4, it seems like something must have changed to cause this but I don't quite know what yet. I'll investigate and fix it.

Thanks for the report b4, it seems like something must have changed to cause this but I don't quite know what yet. I'll investigate and fix it.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
echelon idk zzz
No Assignees idk
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: I2P_Developers/libsam3#2
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?