From patchwork Fri Sep 30 23:10:46 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [1/2] fix gsn_restart file buffer overflow and missing path sep From: Neels Hofmeyr X-Patchwork-Id: 245882 Message-Id: <1475277047-3068-2-git-send-email-nhofmeyr@sysmocom.de> To: osmocom-net-gprs@lists.osmocom.org Date: Sat, 1 Oct 2016 01:10:46 +0200 Fix errors during gsn_restart file path composition: - possible buffer overflow because the wrong remaining length was fed to strncat(). - missing path separator: put restart file in dir/gsn_restart instead of ../dirgsn_restart. This assumes that the path separator is '/'. Use talloc_asprintf() to fix all filename length problems and shorten the code. In order to free the allocated path, add a free_filename label, and jump there instead of returning from the fopen("w") failure branch. Also don't return from "fclose failed" branch in order to free the path, remove the if {} braces. Remove the NAMESIZE constant that is now unused. --- gtp/gtp.c | 18 ++++++++---------- gtp/gtp.h | 1 - 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/gtp/gtp.c b/gtp/gtp.c index 12cb492..161a6f0 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -646,13 +646,11 @@ static void log_restart(struct gsn_t *gsn) FILE *f; int i, rc; int counter = 0; - char filename[NAMESIZE]; - - 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)); - + char *filename; + i = umask(022); + filename = talloc_asprintf(NULL, "%s/%s", gsn->statedir, RESTART_FILE); + OSMO_ASSERT(filename); /* We try to open file. On failure we will later try to create file */ if (!(f = fopen(filename, "r"))) { @@ -680,17 +678,17 @@ static void log_restart(struct gsn_t *gsn) LOGP(DLGTP, LOGL_ERROR, "fopen(path=%s, mode=%s) failed: Error = %s\n", filename, "w", strerror(errno)); - return; + goto free_filename; } umask(i); fprintf(f, "%d\n", gsn->restart_counter); close_file: - if (fclose(f)) { + if (fclose(f)) LOGP(DLGTP, LOGL_ERROR, "fclose failed: Error = %s\n", strerror(errno)); - return; - } +free_filename: + talloc_free(filename); } int gtp_new(struct gsn_t **gsn, char *statedir, struct in_addr *listen, diff --git a/gtp/gtp.h b/gtp/gtp.h index 539e255..1434e1e 100644 --- a/gtp/gtp.h +++ b/gtp/gtp.h @@ -29,7 +29,6 @@ #define ERRMSG_SIZE 255 #define RESTART_FILE "gsn_restart" -#define NAMESIZE 1024 /* GTP version 1 extension header type definitions. */ #define GTP_EXT_PDCP_PDU 0xC0 /* PDCP PDU Number */ From patchwork Fri Sep 30 23:10:47 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [2/2] gsn_restart file: set umask back to original after write failure From: Neels Hofmeyr X-Patchwork-Id: 245883 Message-Id: <1475277047-3068-3-git-send-email-nhofmeyr@sysmocom.de> To: osmocom-net-gprs@lists.osmocom.org Date: Sat, 1 Oct 2016 01:10:47 +0200 An fopen("w") error used to omit the umask() call to reinstate the previous umask. Move the final umask() call to the bottom so that it is called in all code paths. --- gtp/gtp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gtp/gtp.c b/gtp/gtp.c index 161a6f0..a46a76f 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -681,7 +681,6 @@ static void log_restart(struct gsn_t *gsn) goto free_filename; } - umask(i); fprintf(f, "%d\n", gsn->restart_counter); close_file: if (fclose(f)) @@ -689,6 +688,7 @@ close_file: "fclose failed: Error = %s\n", strerror(errno)); free_filename: talloc_free(filename); + umask(i); } int gtp_new(struct gsn_t **gsn, char *statedir, struct in_addr *listen,