ulogd: add +1 char for null char

Submitted by Alexandru Ardelean on March 20, 2017, 8:31 a.m.

Details

Message ID 1489998667-15183-1-git-send-email-ardeleanalex@gmail.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Alexandru Ardelean March 20, 2017, 8:31 a.m.
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(-)

Comments

Eric Leblond March 21, 2017, 8:54 p.m.
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,
Alexandru Ardelean March 22, 2017, 7:07 a.m.
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
Alexandru Ardelean March 22, 2017, 3:56 p.m.
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

Patch hide | download patch | download mbox

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') {