[1/3] Fix possible buffer overflow for gsn_restart file path

Submitted by Neels Hofmeyr on Sept. 15, 2016, 1:06 p.m.

Details

Message ID 1473944800-6054-1-git-send-email-nhofmeyr@sysmocom.de
State New
Series "Series without cover letter"
Headers show

Commit Message

Neels Hofmeyr Sept. 15, 2016, 1:06 p.m.
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/gtp/gtp.c b/gtp/gtp.c
index 12cb492..55a8ce4 100644
--- a/gtp/gtp.c
+++ b/gtp/gtp.c
@@ -650,7 +650,7 @@  static void log_restart(struct gsn_t *gsn)
 
 	filename[NAMESIZE - 1] = 0;	/* No null term. guarantee by strncpy */
 	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);
 

Comments

Neels Hofmeyr Sept. 15, 2016, 1:12 p.m.
This is a patch series for OpenGGSN.  It was submitted before on August 18th,
but this new version separates the nul termination thing to a separate commit
(and slightly tweaks it).

~Neels

On Thu, Sep 15, 2016 at 03:06:38PM +0200, Neels Hofmeyr 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.
[...]
Holger Freyther Sept. 15, 2016, 1:37 p.m.
> On 15 Sep 2016, at 15:12, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> This is a patch series for OpenGGSN.  It was submitted before on August 18th,
> but this new version separates the nul termination thing to a separate commit
> (and slightly tweaks it).

I think I had raised it before. The code is not performance critical on start so why not use talloc_strdup here?

holger
Neels Hofmeyr Sept. 16, 2016, 10:58 a.m.
On Thu, Sep 15, 2016 at 03:37:35PM +0200, Holger Freyther wrote:
> I think I had raised it before. The code is not performance critical on start so why not use talloc_strdup here?

I must have missed that somehow. Makes more sense indeed.

~Neels