Re: [PATCH] ulogd: add +1 char for null char

[Alexandru Ardelean]

On Wed, Mar 22, 2017 at 9:07 AM, Alexandru Ardelean
 wrote:
> On Tue, Mar 21, 2017 at 10:54 PM, Eric Leblond  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 
>
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ulogd: add +1 char for null char

[Alexandru Ardelean]

On Tue, Mar 21, 2017 at 10:54 PM, Eric Leblond  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 

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ulogd: add +1 char for null char

[Eric Leblond]

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 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ulogd: add +1 char for null char

[Alexandru Ardelean]

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 
---
 output/sqlite3/ulogd_output_SQLITE3.c | 6 +++---
 src/ulogd.c   | 2 +-
 util/db.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

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

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html