diff mbox

[openggsn,1/2] Fix possible buffer overflow for gsn_restart file path

Message ID 1471485975-1775-1-git-send-email-nhofmeyr@sysmocom.de
State New
Headers show

Commit Message

Neels Hofmeyr Aug. 18, 2016, 2:06 a.m. UTC 
For strncat, to obtain n, one must not subtract the length of what is appended,
but the length of what is already written from the buffer size.

Verified with this little test program:

 #include <stdio.h>
 #include <string.h>

 int main() {
   char buf[20];
   strncpy(buf, "123", 10);
   strncat(buf, "456789012345", 10 - strlen(buf));
   printf("%s\n", buf);
   return 0;
 }

It prints "1234567890".
---
 gtp/gtp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Holger Freyther Aug. 18, 2016, 8:42 a.m. UTC | #1 
> On 18 Aug 2016, at 04:06, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> For strncat, to obtain n, one must not subtract the length of what is appended,
> but the length of what is already written from the buffer size.
> 

let's use talloc_asprintf or the append variant of it. The place doesn't look like performance critical code.

holger
diff mbox

Patch

diff --git a/gtp/gtp.c b/gtp/gtp.c
index 12cb492..34e1dc6 100644
--- a/gtp/gtp.c
+++ b/gtp/gtp.c
@@ -648,9 +648,10 @@  static void log_restart(struct gsn_t *gsn)
 	int counter = 0;
 	char filename[NAMESIZE];
 
-	filename[NAMESIZE - 1] = 0;	/* No null term. guarantee by strncpy */
+	/* guarantee nul term, strncpy might omit if too long */
+	filename[NAMESIZE - 1] = 0;
 	strncpy(filename, gsn->statedir, NAMESIZE - 1);
-	strncat(filename, RESTART_FILE, NAMESIZE - 1 - sizeof(RESTART_FILE));
+	strncat(filename, RESTART_FILE, NAMESIZE - 1 - strlen(filename));
 
 	i = umask(022);