| Message ID | 1489998667-15183-1-git-send-email-ardeleanalex@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Hello, Thanks for the report and the patch. I'm not sure of your implementation. Can you test with the patch to follow ? On Mon, 2017-03-20 at 10:31 +0200, Alexandru Ardelean wrote: > This is a bit zealous to fix like this, but it seems to work. > > The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16. > > And also on LEDE (mips_24kc and ARM): > https://github.com/openwrt/packages/issues/4123 > https://github.com/openwrt/packages/issues/4090 > > I personally saw it on ppc32. > The offending code was in `pluginstance_alloc_init()` line 671: > ``` > memcpy(pi->id, pi_id, sizeof(pi->id)); > ``` Thanks in advance,
On Tue, Mar 21, 2017 at 10:54 PM, Eric Leblond <eric@regit.org> wrote: > Hello, > > Thanks for the report and the patch. I'm not sure of your > implementation. Can you test with the patch to follow ? > > On Mon, 2017-03-20 at 10:31 +0200, Alexandru Ardelean wrote: >> This is a bit zealous to fix like this, but it seems to work. >> >> The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16. >> >> And also on LEDE (mips_24kc and ARM): >> https://github.com/openwrt/packages/issues/4123 >> https://github.com/openwrt/packages/issues/4090 >> >> I personally saw it on ppc32. >> The offending code was in `pluginstance_alloc_init()` line 671: >> ``` >> memcpy(pi->id, pi_id, sizeof(pi->id)); >> ``` > > Thanks in advance, > -- > Eric Leblond <eric@regit.org> Hey, Thanks for the response. Will test out your patch & come back. After submitting mine, there was a similar discussion on one of the Github threads [to use strncpy()/strlcpy()]. A few questions: 1) would strlcpy(d,s,len) be better [here & in general] ? [since it guarantees a null-char at the end of `len`] ? 1a) maybe it could be considered to replace strncpy() in more places [where the case is appropriate] 2) any thoughts on sanitizing the use of ULOGD_MAX_KEYLEN ? ; general gist of it would be #define ULOGD_MAX_KEYLEN 32 and remove any `ULOGD_MAX_KEYLEN+1` or `ULOGD_MAX_KEYLEN-1` ; which sort of seemed confusing ; and combined with strlcpy() it should give an overall more robust approach Thanks Alex -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 22, 2017 at 9:07 AM, Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Tue, Mar 21, 2017 at 10:54 PM, Eric Leblond <eric@regit.org> wrote: >> Hello, >> >> Thanks for the report and the patch. I'm not sure of your >> implementation. Can you test with the patch to follow ? >> >> On Mon, 2017-03-20 at 10:31 +0200, Alexandru Ardelean wrote: >>> This is a bit zealous to fix like this, but it seems to work. >>> >>> The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16. >>> >>> And also on LEDE (mips_24kc and ARM): >>> https://github.com/openwrt/packages/issues/4123 >>> https://github.com/openwrt/packages/issues/4090 >>> >>> I personally saw it on ppc32. >>> The offending code was in `pluginstance_alloc_init()` line 671: >>> ``` >>> memcpy(pi->id, pi_id, sizeof(pi->id)); >>> ``` >> >> Thanks in advance, >> -- >> Eric Leblond <eric@regit.org> > > Hey, > > Thanks for the response. > Will test out your patch & come back. > > After submitting mine, there was a similar discussion on one of the > Github threads [to use strncpy()/strlcpy()]. > > A few questions: > 1) would strlcpy(d,s,len) be better [here & in general] ? [since it > guarantees a null-char at the end of `len`] ? > 1a) maybe it could be considered to replace strncpy() in more places > [where the case is appropriate] > 2) any thoughts on sanitizing the use of ULOGD_MAX_KEYLEN ? ; general > gist of it would be #define ULOGD_MAX_KEYLEN 32 and remove any > `ULOGD_MAX_KEYLEN+1` or `ULOGD_MAX_KEYLEN-1` ; which sort of seemed > confusing ; and combined with strlcpy() it should give an overall more > robust approach > > Thanks > Alex on a more punctual note your patch works :) feel free to push regarding my comments, i don't have strong opinions ; i'll leave it up to you thanks Alex -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c index 20ceb3b..ea66061 100644 --- a/output/sqlite3/ulogd_output_SQLITE3.c +++ b/output/sqlite3/ulogd_output_SQLITE3.c @@ -48,7 +48,7 @@ struct field { TAILQ_ENTRY(field) link; - char name[ULOGD_MAX_KEYLEN]; + char name[ULOGD_MAX_KEYLEN+1]; /* +1 for null char */ struct ulogd_key *key; }; @@ -214,7 +214,7 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi) { struct sqlite3_priv *priv = (void *)pi->private; struct field *f; - char buf[ULOGD_MAX_KEYLEN]; + char buf[ULOGD_MAX_KEYLEN+1]; /* +1 for null char */ char *underscore; char *stmt_pos; int i, cols = 0; @@ -305,7 +305,7 @@ static int sqlite3_init_db(struct ulogd_pluginstance *pi) { struct sqlite3_priv *priv = (void *)pi->private; - char buf[ULOGD_MAX_KEYLEN]; + char buf[ULOGD_MAX_KEYLEN+1]; char *underscore; struct field *f; sqlite3_stmt *schema_stmt; diff --git a/src/ulogd.c b/src/ulogd.c index 5b9a586..0d6a367 100644 --- a/src/ulogd.c +++ b/src/ulogd.c @@ -942,7 +942,7 @@ static int create_stack(const char *option) /* PASS 1: find and instanciate plugins of stack, link them together */ for (tok = strtok(buf, ",\n"); tok; tok = strtok(NULL, ",\n")) { char *plname, *equals; - char pi_id[ULOGD_MAX_KEYLEN]; + char pi_id[ULOGD_MAX_KEYLEN+1]; /* +1 for the null char */ struct ulogd_pluginstance *pi; struct ulogd_plugin *pl; diff --git a/util/db.c b/util/db.c index c9aec41..6af4555 100644 --- a/util/db.c +++ b/util/db.c @@ -96,7 +96,7 @@ static int sql_createstmt(struct ulogd_pluginstance *upi) if (strncasecmp(procedure,"INSERT", strlen("INSERT")) == 0 && (procedure[strlen("INSERT")] == '\0' || procedure[strlen("INSERT")] == ' ')) { - char buf[ULOGD_MAX_KEYLEN]; + char buf[ULOGD_MAX_KEYLEN+1]; /* +1 for null char */ char *underscore; if(procedure[6] == '\0') {
This is a bit zealous to fix like this, but it seems to work. The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16. And also on LEDE (mips_24kc and ARM): https://github.com/openwrt/packages/issues/4123 https://github.com/openwrt/packages/issues/4090 I personally saw it on ppc32. The offending code was in `pluginstance_alloc_init()` line 671: ``` memcpy(pi->id, pi_id, sizeof(pi->id)); ``` Seems that it would copy 1 char from the stack, and that caused some failsafes to kick in. This fix addresses the issue directly. Maybe a more appropriate rework of string stuff would be needed. What I also noticed, is that there's also places in the code that define name[ULOGD_MAX_KEYLEN+1] and some that don't add the +1 char. Basically, this just aligns the remaining bits of code that don't add the +1 char. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> --- output/sqlite3/ulogd_output_SQLITE3.c | 6 +++--- src/ulogd.c | 2 +- util/db.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)