[PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
strlcpy() implementations typically scan for the end of the source argument
passed to strlcpy(). Hence avoid passing an unterminated string to strlcpy().
Reported-by: Stuart Henderson
Fixes: 7f05daa8e0e0 ("CHANGES: BUG: 3444939: BUG: 1796886: snmplib: Avoid that
sprint_realloc_octet_string() embeds unprintable control characters or binary
zeroes in its output. This behavior could cause truncated output in snmptrapd.")
---
snmplib/mib.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/snmplib/mib.c b/snmplib/mib.c
index a16144065029..1c875c06464f 100644
--- a/snmplib/mib.c
+++ b/snmplib/mib.c
@@ -585,9 +585,10 @@ sprint_realloc_octet_string(u_char ** buf, size_t *
buf_len,
return 0;
}
if (memchr(cp, '\0', cnt) == NULL) {
-/* No embedded '\0' - use strlcpy() to preserve UTF-8
*/
-strlcpy((char *)(*buf + *out_len), (char *)cp, cnt +
1);
+/* No embedded '\0' - use memcpy() to preserve UTF-8 */
+memcpy(*buf + *out_len, cp, cnt);
*out_len += cnt;
+*(*buf + *out_len) = '\0';
} else if (!sprint_realloc_asciistring(buf, buf_len,
out_len, allow_realloc, cp, cnt)) {
return 0;
--
2.16.3
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On 2018/05/19 14:07, Bart Van Assche wrote:
> strlcpy() implementations typically scan for the end of the source argument
> passed to strlcpy(). Hence avoid passing an unterminated string to strlcpy().
>
> Reported-by: Stuart Henderson
> Fixes: 7f05daa8e0e0 ("CHANGES: BUG: 3444939: BUG: 1796886: snmplib: Avoid
> that sprint_realloc_octet_string() embeds unprintable control characters or
> binary zeroes in its output. This behavior could cause truncated output in
> snmptrapd.")
Confirming that (as expected) this fixes things for me, thanks Bart.
> ---
> snmplib/mib.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/snmplib/mib.c b/snmplib/mib.c
> index a16144065029..1c875c06464f 100644
> --- a/snmplib/mib.c
> +++ b/snmplib/mib.c
> @@ -585,9 +585,10 @@ sprint_realloc_octet_string(u_char ** buf, size_t *
> buf_len,
> return 0;
> }
> if (memchr(cp, '\0', cnt) == NULL) {
> -/* No embedded '\0' - use strlcpy() to preserve
> UTF-8 */
> -strlcpy((char *)(*buf + *out_len), (char *)cp, cnt +
> 1);
> +/* No embedded '\0' - use memcpy() to preserve UTF-8
> */
> +memcpy(*buf + *out_len, cp, cnt);
> *out_len += cnt;
> +*(*buf + *out_len) = '\0';
> } else if (!sprint_realloc_asciistring(buf, buf_len,
> out_len, allow_realloc, cp, cnt)) {
> return 0;
> --
> 2.16.3
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Sat, May 19, 2018 at 02:07:56PM -0700, Bart Van Assche wrote:
> strlcpy() implementations typically scan for the end of the source argument
> passed to strlcpy(). Hence avoid passing an unterminated string to strlcpy().
>
> Reported-by: Stuart Henderson
> Fixes: 7f05daa8e0e0 ("CHANGES: BUG: 3444939: BUG: 1796886: snmplib: Avoid
> that sprint_realloc_octet_string() embeds unprintable control characters or
> binary zeroes in its output. This behavior could cause truncated output in
> snmptrapd.")
> ---
> snmplib/mib.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/snmplib/mib.c b/snmplib/mib.c
> index a16144065029..1c875c06464f 100644
> --- a/snmplib/mib.c
> +++ b/snmplib/mib.c
> @@ -585,9 +585,10 @@ sprint_realloc_octet_string(u_char ** buf, size_t *
> buf_len,
> return 0;
> }
> if (memchr(cp, '\0', cnt) == NULL) {
> -/* No embedded '\0' - use strlcpy() to preserve
> UTF-8 */
> -strlcpy((char *)(*buf + *out_len), (char *)cp, cnt +
> 1);
> +/* No embedded '\0' - use memcpy() to preserve UTF-8
> */
The comment does not make any sense - what have UTF-8 got to do with this?
Anyway, I went and looked on the spec for the 't' display hint in rfc2579
ยง3.1 and it explicitly states that unterminated utf-8 characters at the end
of an t encoded octet string are to be discarded.
Looking at the code surrounding this chunk seems to suggest that some TLC for
the 't' display hint could be useful.
> +memcpy(*buf + *out_len, cp, cnt);
In any case I have to admit that the functional part of the patch (use memcpy
rather than strlcpy to copy possibly not null terminated data) is good so
+1 for this part
/MF
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On 05/19/18 15:14, Magnus Fromreide wrote:
On Sat, May 19, 2018 at 02:07:56PM -0700, Bart Van Assche wrote:
strlcpy() implementations typically scan for the end of the source argument
passed to strlcpy(). Hence avoid passing an unterminated string to strlcpy().
Reported-by: Stuart Henderson
Fixes: 7f05daa8e0e0 ("CHANGES: BUG: 3444939: BUG: 1796886: snmplib: Avoid that
sprint_realloc_octet_string() embeds unprintable control characters or binary zeroes in
its output. This behavior could cause truncated output in snmptrapd.")
---
snmplib/mib.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/snmplib/mib.c b/snmplib/mib.c
index a16144065029..1c875c06464f 100644
--- a/snmplib/mib.c
+++ b/snmplib/mib.c
@@ -585,9 +585,10 @@ sprint_realloc_octet_string(u_char ** buf, size_t *
buf_len,
return 0;
}
if (memchr(cp, '\0', cnt) == NULL) {
-/* No embedded '\0' - use strlcpy() to preserve UTF-8
*/
-strlcpy((char *)(*buf + *out_len), (char *)cp, cnt +
1);
+/* No embedded '\0' - use memcpy() to preserve UTF-8 */
The comment does not make any sense - what have UTF-8 got to do with this?
Hello Magnus,
Does this mean that you did not understand that comment? The code under
the else-clause does not preserve UTF-8 (sprint_realloc_asciistring()).
Hence the comment in the if-clause about preserving UTF-8.
Bart.
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Sat, 19 May 2018 14:07:56 -0700 Bart wrote: BVA> strlcpy() implementations typically scan for the end of the BVA> source argument passed to strlcpy(). Hence avoid passing an BVA> unterminated string to strlcpy(). I'm going to say -1, but for the patch and not the need to fix the issue. This code is treating 'a' and 't' hints the same. There should never be UTF-8 in an ascii string. Why not revert back to the original code for 'a' case and put UTF-8 handling where it belongs, under the 't' case. Robert -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Sat, May 19, 2018 at 05:28:31PM -0700, Bart Van Assche wrote:
> On 05/19/18 15:14, Magnus Fromreide wrote:
> > On Sat, May 19, 2018 at 02:07:56PM -0700, Bart Van Assche wrote:
> > > strlcpy() implementations typically scan for the end of the source
> > > argument
> > > passed to strlcpy(). Hence avoid passing an unterminated string to
> > > strlcpy().
> > >
> > > Reported-by: Stuart Henderson
> > > Fixes: 7f05daa8e0e0 ("CHANGES: BUG: 3444939: BUG: 1796886: snmplib: Avoid
> > > that sprint_realloc_octet_string() embeds unprintable control characters
> > > or binary zeroes in its output. This behavior could cause truncated
> > > output in snmptrapd.")
> > > ---
> > > snmplib/mib.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/snmplib/mib.c b/snmplib/mib.c
> > > index a16144065029..1c875c06464f 100644
> > > --- a/snmplib/mib.c
> > > +++ b/snmplib/mib.c
> > > @@ -585,9 +585,10 @@ sprint_realloc_octet_string(u_char ** buf, size_t *
> > > buf_len,
> > > return 0;
> > > }
> > > if (memchr(cp, '\0', cnt) == NULL) {
> > > -/* No embedded '\0' - use strlcpy() to preserve
> > > UTF-8 */
> > > -strlcpy((char *)(*buf + *out_len), (char *)cp,
> > > cnt + 1);
> > > +/* No embedded '\0' - use memcpy() to preserve
> > > UTF-8 */
> >
> > The comment does not make any sense - what have UTF-8 got to do with this?
>
> Hello Magnus,
>
> Does this mean that you did not understand that comment?
The comment is clear enough and I can even see how it came to be but it
really is ascii that is the special case here as it replaces everything
that isn't printable or a space in the current locale with a dot (I have
to admit that I see that as odd as well).
> The code under the else-clause does not preserve UTF-8
> (sprint_realloc_asciistring()). Hence the comment in the if-clause about
> preserving UTF-8.
> Bart.
>
>
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Net-snmp-coders mailing list
> Net-snmp-coders@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On 05/19/18 21:18, Robert Story wrote: On Sat, 19 May 2018 14:07:56 -0700 Bart wrote: BVA> strlcpy() implementations typically scan for the end of the BVA> source argument passed to strlcpy(). Hence avoid passing an BVA> unterminated string to strlcpy(). I'm going to say -1, but for the patch and not the need to fix the issue. This code is treating 'a' and 't' hints the same. There should never be UTF-8 in an ascii string. Why not revert back to the original code for 'a' case and put UTF-8 handling where it belongs, under the 't' case. Please have a look at the original bug reports. These reports apply to UTF-8 text stored in fields that have an 'a' (ASCII) display hint. Bart. -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Sun, May 20, 2018 at 12:18 AM, Robert Story wrote: > On Sat, 19 May 2018 14:07:56 -0700 Bart wrote: > BVA> strlcpy() implementations typically scan for the end of the > BVA> source argument passed to strlcpy(). Hence avoid passing an > BVA> unterminated string to strlcpy(). > > I'm going to say -1, but for the patch and not the need to fix the > issue. > > This code is treating 'a' and 't' hints the same. There should > never be UTF-8 in an ascii string. Why not revert back to the > original code for 'a' case and put UTF-8 handling where it belongs, > under the 't' case. > There's a much longer discussion to be had here, one hint at how complex the discussion will get is that RFC2571 says that SnmpAdminString is "encoded as an octet string using the UTF-8 transformation" and has a DISPLAY-HINT of "255a". So, does "a" mean UTF-8 or ASCII or is it fundamentally a little indeterminate due to ancient mistakes and current conventions? My personal feeling is that if a string is valid UTF-8 (which can be determined in one pass using the algorithm I pointed to in the bug), it should be output as-is, whether the DISPLAY-HINT is "a" or "t". We should protect against not just NUL bytes as the current code does but also control characters (e.g., if there's an embedded "\n" in the string, will that foil a future log parser that's trying to do line-based parsing of a log file?). I do not think that now is the time to try to deal with any of the fundamentals, but just not regress from previous released behavior, and deal with the underlying issue in 5.8.1 / 5.7.4. Bill -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Sun, 20 May 2018 13:06:43 -0400 Bill wrote: BF> I do not think that now is the time to try to deal with any of BF> the fundamentals, but just not regress from previous released BF> behavior, and deal with the underlying issue in 5.8.1 / 5.7.4. Just to clarify, do you mean a) accepts Bart's RFV patch to fix the UTF-8 patch or b) revert the UTF-8 patch entirely an deal with the issue after a fuller discussion? Robert -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Sun, May 20, 2018 at 2:25 PM, Robert Story wrote:
> On Sun, 20 May 2018 13:06:43 -0400 Bill wrote:
> BF> I do not think that now is the time to try to deal with any of
> BF> the fundamentals, but just not regress from previous released
> BF> behavior, and deal with the underlying issue in 5.8.1 / 5.7.4.
>
> Just to clarify, do you mean
>
> a) accepts Bart's RFV patch to fix the UTF-8 patch
>
> or
>
> b) revert the UTF-8 patch entirely an deal with the issue after a
> fuller discussion?
>
> If these are my only two options, I pick (a), since (b) results in a
regression from 5.7.1's behavior.
The original code was:
-for (x = 0; x < width && cp < ecp; x++) {
-*(*buf + *out_len) = *cp++;
-(*out_len)++;
}
-*(*buf + *out_len) = '\0';
which is not that different from the memcpy() that Bart's patch adds.
So, +1 on committing Bart's patch, because it accomplishes the goal of
fixing the regression, with the caveat that I really think that this whole
area needs to be revisited. My fundamental proposal is to use the one-pass
"is this utf8" predicate and "if so" memcpy, and "if not" use the ascii
print. This ignores for now the question of an octet string that is
truncated in the middle of a UTF-8 sequence (the "t" format says "output
all the well-formed UTF-8 sequences", and my proposal would print it as
ascii with all the "."s, but we can talk about that later. It's possible
that we could modify the "is this utf8" to say "give me the prefix that is
utf8", to fix the truncation issue.
Bill
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
Bill Fenner writes: > So, +1 on committing Bart's patch, because it accomplishes the goal of > fixing the regression, with the caveat that I really think that this > whole area needs to be revisited. I think this comment is spot on. So I'd accept the patch too, though my gut reaction is to revert to whatever was in 5.7.1 because of the late point in this release cycle and we're redsigning critical code on the fly right before a release. We really need a test C function to check this stuff. Any volunteers? (I'm not a UTF-8 expert at all) -- Wes Hardaker Please mail all replies to net-snmp-coders@lists.sourceforge.net -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Mon, May 21, 2018 at 11:18 AM, Wes Hardaker < harda...@users.sourceforge.net> wrote: > Bill Fenner writes: > > > So, +1 on committing Bart's patch, because it accomplishes the goal of > > fixing the regression, with the caveat that I really think that this > > whole area needs to be revisited. > > I think this comment is spot on. So I'd accept the patch too, though my > gut reaction is to revert to whatever was in 5.7.1 because of the late > point in this release cycle and we're redsigning critical code on the > fly right before a release. > > We really need a test C function to check this stuff. Any volunteers? > (I'm not a UTF-8 expert at all) The one I alluded to in an earlier email: http://bjoern.hoehrmann.de/utf-8/decoder/dfa/ If we accept the dense magic, the code can be used for both "is this string valid utf8" and "print all the valid utf8 if the last utf8-encoded character got cut off on an octet boundary". Bill -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [PATCH; request for votes] snmplib: Avoid that sprint_realloc_octet_string() triggers a segmentation fault
On Mon, May 21, 2018 at 02:36:43PM -0400, Bill Fenner wrote: > On Mon, May 21, 2018 at 11:18 AM, Wes Hardaker < > harda...@users.sourceforge.net> wrote: > > > Bill Fenner writes: > > > > > So, +1 on committing Bart's patch, because it accomplishes the goal of > > > fixing the regression, with the caveat that I really think that this > > > whole area needs to be revisited. > > > > I think this comment is spot on. So I'd accept the patch too, though my > > gut reaction is to revert to whatever was in 5.7.1 because of the late > > point in this release cycle and we're redsigning critical code on the > > fly right before a release. > > > > We really need a test C function to check this stuff. Any volunteers? > > (I'm not a UTF-8 expert at all) > > > The one I alluded to in an earlier email: > http://bjoern.hoehrmann.de/utf-8/decoder/dfa/ > > If we accept the dense magic, the code can be used for both "is this string > valid utf8" and "print all the valid utf8 if the last utf8-encoded > character got cut off on an octet boundary". Interesting piece of code. What I am pondering is more what the 'a' encoding should do. My reading of the rfc says that the display-hint "1t" should display all ascii characters and suppress anything else while a display-hint of "1a" or "255a" is a lot less specified. If we assume a string containing iso-8859-1 (Latin 1) data then "1t" should reject it (unless we are unlucky) and it is unclear what an "a" display-hint should do with it. > Bill > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > ___ > Net-snmp-coders mailing list > Net-snmp-coders@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/net-snmp-coders -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
