Send inn-workers mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.isc.org/mailman/listinfo/inn-workers
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of inn-workers digest..."
Today's Topics:
1. Re: INN and openssl 1.1 (Julien ?LIE)
2. Re: INN and openssl 1.1
(Dave Shariff Yadallee - System Administrator a.k.a. The Root of the
Problem)
3. potential buffer overrun in innd/art.c due to strlcpy misuse
(Paul Eggert)
4. Re: The semantics of strlcpy and strlcat (Paul Eggert)
5. Re: potential buffer overrun in innd/art.c due to strlcpy
misuse (Russ Allbery)
----------------------------------------------------------------------
Message: 1
Date: Sat, 23 Jan 2016 14:55:04 +0100
From: Julien ?LIE <[email protected]>
To: [email protected]
Subject: Re: INN and openssl 1.1
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi The Doctor,
> Only some minor tweeks are needed for Openssl 1.1 compatability AFAIK.
Thanks for having tested INN against the alpha version of OpenSSL 1.1.0.
> So 496 to 498 Currently read
>
> SSLeay_add_ssl_algorithms();
>
> CTX = SSL_CTX_new(SSLv23_server_method());
>
> For Openssl 1.1 they would need to read
>
> OpenSSL_add_ssl_algorithms();
>
> CTX = SSL_CTX_new(TLS_server_method());
>
> Hopefully OPenssl commiter for 1.1 branch will hear my plea for
> backwards compatability so that you have
>
> #define SSLeay_add_ssl_algorithms OpenSSL_add_ssl_algorithms
> #define SSLv23_server_method TLS_server_method
It would still need to build OpenSSL with OPENSSL_USE_DEPRECATED, which
is not always the case, so we shouldn't rely on that.
According to a previous thread on that subject on the OpenSSL
mailing-list
<https://mta.openssl.org/pipermail/openssl-dev/2015-May/001449.html> it
seems the best fix for INN would be to use something like:
+#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
+ CTX = SSL_CTX_new(TLS_client_method());
+#else
CTX = SSL_CTX_new(SSLv23_client_method());
+#endif
Regarding SSLeay_add_ssl_algorithms(), we could have used
SSL_library_init() instead since OpenSSL 0.9.6 (in 2001).
I also see that OpenSSL now has SSL_set_min_proto_version to define the
lowest permitted SSL/TLS protocol version.
https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_min_proto_version.html
We should consider (for a future release) changing how the current
tlsprotocols: keyword in inn.conf work.
tlsprotocols: [ TLSv1 TLSv1.1 TLSv1.2 ]
could instead be:
tlsminprotocol: TLSv1
The advantage is that INN won't remove support for TLSv1.3 or TLSv2.0
when they have been released (in the almost certain case that the news
admin does not add TLSv1.3 or TLSv2.0 in the tlsprotocols: inn.conf
keyword!).
I think we should do that move (with innupgrade) for INN 2.6.1 and not
wait for INN 2.7.0.
--
Julien ?LIE
? Si ?a n'a pas fait boum, c'est peut-?tre le succ?s ? ?
(Ast?rix)
------------------------------
Message: 2
Date: Sat, 23 Jan 2016 07:09:33 -0700
From: "Dave Shariff Yadallee - System Administrator a.k.a. The Root of
the Problem" <[email protected]>
To: Julien ?LIE <[email protected]>
Cc: [email protected]
Subject: Re: INN and openssl 1.1
Message-ID: <[email protected]>
Content-Type: text/plain; charset=iso-8859-1
On Sat, Jan 23, 2016 at 02:55:04PM +0100, Julien ?LIE wrote:
> Hi The Doctor,
>
> >Only some minor tweeks are needed for Openssl 1.1 compatability AFAIK.
>
> Thanks for having tested INN against the alpha version of OpenSSL 1.1.0.
>
>
>
> >So 496 to 498 Currently read
> >
> > SSLeay_add_ssl_algorithms();
> >
> > CTX = SSL_CTX_new(SSLv23_server_method());
> >
> >For Openssl 1.1 they would need to read
> >
> > OpenSSL_add_ssl_algorithms();
> >
> > CTX = SSL_CTX_new(TLS_server_method());
> >
> >Hopefully OPenssl commiter for 1.1 branch will hear my plea for
> >backwards compatability so that you have
> >
> >#define SSLeay_add_ssl_algorithms OpenSSL_add_ssl_algorithms
> >#define SSLv23_server_method TLS_server_method
>
> It would still need to build OpenSSL with OPENSSL_USE_DEPRECATED, which is
> not always the case, so we shouldn't rely on that.
>
> According to a previous thread on that subject on the OpenSSL mailing-list
> <https://mta.openssl.org/pipermail/openssl-dev/2015-May/001449.html> it
> seems the best fix for INN would be to use something like:
>
> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
> + CTX = SSL_CTX_new(TLS_client_method());
> +#else
> CTX = SSL_CTX_new(SSLv23_client_method());
> +#endif
>
>
>
> Regarding SSLeay_add_ssl_algorithms(), we could have used SSL_library_init()
> instead since OpenSSL 0.9.6 (in 2001).
>
>
>
> I also see that OpenSSL now has SSL_set_min_proto_version to define the
> lowest permitted SSL/TLS protocol version.
>
> https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_min_proto_version.html
>
> We should consider (for a future release) changing how the current
> tlsprotocols: keyword in inn.conf work.
> tlsprotocols: [ TLSv1 TLSv1.1 TLSv1.2 ]
> could instead be:
> tlsminprotocol: TLSv1
>
> The advantage is that INN won't remove support for TLSv1.3 or TLSv2.0 when
> they have been released (in the almost certain case that the news admin does
> not add TLSv1.3 or TLSv2.0 in the tlsprotocols: inn.conf keyword!).
> I think we should do that move (with innupgrade) for INN 2.6.1 and not wait
> for INN 2.7.0.
>
> --
> Julien ?LIE
>
> ? Si ?a n'a pas fait boum, c'est peut-?tre le succ?s ? ?
> (Ast?rix)
> _______________________________________________
> inn-workers mailing list
> [email protected]
> https://lists.isc.org/mailman/listinfo/inn-workers
I fully concur / Je suis d'accord.
This makes things easier.
INN so far is the only package against Openssl 1.1 that is easy to migrate.
--
For effective Internet Etiquette and communications read
http://catb.org/jargon/html/T/top-post.html, http://idallen.com/topposting.html
& http://www.caliburn.nl/topposting.html
------------------------------
Message: 3
Date: Sat, 23 Jan 2016 03:04:45 -0800
From: Paul Eggert <[email protected]>
To: [email protected]
Subject: potential buffer overrun in innd/art.c due to strlcpy misuse
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"; Format="flowed"
Russ Allbery mentioned that INN uses strlcpy and strlcat, and I looked through
the code by hand to see how well that was working out. I noticed that all uses
ignored the returned value, except for one place in innd/art.c. Unfortunately
that usage assumes that strlcpy returns strlen(dst) afterwards, but strlcpy
actually returns strlen(src). This looks like it could lead to a buffer
overrun.
Also, the code does not appear to ensure that the result is null-terminated
(this is due to its appending ' ' without checking whether the space fits).
Proposed untested patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inn.diff
Type: text/x-diff
Size: 789 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20160123/082fd7aa/attachment-0001.bin>
------------------------------
Message: 4
Date: Sat, 23 Jan 2016 03:24:28 -0800
From: Paul Eggert <[email protected]>
To: Russ Allbery <[email protected]>, Zack Weinberg <[email protected]>
Cc: [email protected]
Subject: Re: The semantics of strlcpy and strlcat
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"; Format="flowed"
[This message is responding to
<https://sourceware.org/ml/libc-alpha/2016-01/msg00573.html>. I am removing
libc-alpha from CC:, and adding inn-workers to CC:, since this message is
INN-specific rather than being a glibc thing.]
Russ Allbery wrote:
> I'd like to remove them all from INN as well. Unfortunately, INN is a
> very old code base that's riddled with buffers of fixed size allocated off
> the stack with unclear object lifetimes and no further thought given to
> any form of memory management, which makes it hard to take the approach
> that I'm using with all other software I maintain (switching to asprintf).
> Short of a grand removal of static buffers that's an immense amount of
> work and may not happen for software that's mostly in maintenance mode,
> I'm not sure what the best solution is. (I stand by my original feeling
> that silent truncation is generally better than silent buffer overflows,
> but that's a little like saying that tuberculosis is better than ebola.)
Looking at INN it appears that strlcpy and strlcat are used as
belt-and-suspenders copiers -- callers do not check for truncation (except in
one place, where a caller checks for truncation incorrectly and this apparently
can lead to buffer overflow -- I just now filed a bug report about that to
inn-workers).
If so, how about the attached patch? It assumes the fix I sent earlier for the
truncation bug. The basic idea is that truncations should never occur, but if
any does occur then it should be caught and logged. I haven't tested this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inn1.diff
Type: text/x-diff
Size: 4725 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20160123/7d8ef4ef/attachment-0001.bin>
------------------------------
Message: 5
Date: Sat, 23 Jan 2016 16:57:21 -0800
From: Russ Allbery <[email protected]>
To: [email protected], Paul Eggert <[email protected]>
Subject: Re: potential buffer overrun in innd/art.c due to strlcpy
misuse
Message-ID: <[email protected]>
Content-Type: text/plain
Paul Eggert <[email protected]> writes:
> Russ Allbery mentioned that INN uses strlcpy and strlcat, and I looked
> through the code by hand to see how well that was working out. I noticed
> that all uses ignored the returned value, except for one place in
> innd/art.c. Unfortunately that usage assumes that strlcpy returns
> strlen(dst) afterwards, but strlcpy actually returns strlen(src). This
> looks like it could lead to a buffer overrun. Also, the code does not
> appear to ensure that the result is null-terminated (this is due to its
> appending ' ' without checking whether the space fits).
> Proposed untested patch attached.
It turns out that this couldn't cause a buffer overflow because some
entirely separate part of the code initializes the buffer to the maximum
possible size that this code could assemble (there are a lot of
unfortunate patterns like that in the source code), but indeed this is a
misuse of the strlcpy function.
This whole section of code is unnecessary, since it's manipulating a
buffer, which is a dynamically-resizeable structure if you use the correct
API. Sigh.
I think this is the correct fix, which gets rid of all use of these
functions in the FNLnames code and uses the buffer correctly. It requires
a bit of fixing up, since other parts of the code incorrectly used the
"used" struct member instead of the "left" struct member to figure out the
length of the data in the buffer.
I don't have any good way to test this at the moment (have no test server
setup myself at present -- I keep meaning to fix that, but haven't had
much time), but I'm fairly sure that this is correct. I'm going to go
ahead and commit it to CURRENT. Yell at me if it breaks anything.
Thank you very much for looking at this, Paul!
Index: innd/art.c
===================================================================
--- innd/art.c (revision 9981)
+++ innd/art.c (working copy)
@@ -1670,7 +1670,6 @@
SITE *sp, *funnel;
int i, j, Groupcount, Followcount, Crosscount;
char *p, *q, *begin, savec;
- struct buffer *bp;
bool sendit;
/* Work out which sites should really get it. */
@@ -1813,13 +1812,9 @@
funnel = &Sites[sp->Funnel];
funnel->Sendit = true;
if (funnel->FNLwantsnames) {
- bp = &funnel->FNLnames;
- p = &bp->data[bp->used];
- if (bp->used) {
- *p++ = ' ';
- bp->used++;
- }
- bp->used += strlcpy(p, sp->Name, bp->size - bp->used);
+ if (funnel->FNLnames.left != 0)
+ buffer_append(&funnel->FNLnames, " ", 1);
+ buffer_append(&funnel->FNLnames, sp->Name, strlen(sp->Name));
}
}
}
@@ -2181,7 +2176,7 @@
sp->Poison = false;
sp->Sendit = false;
sp->Seenit = false;
- sp->FNLnames.used = 0;
+ buffer_set(&sp->FNLnames, NULL, 0);
sp->ng = NULL;
}
Index: innd/newsfeeds.c
===================================================================
--- innd/newsfeeds.c (revision 9981)
+++ innd/newsfeeds.c (working copy)
@@ -842,14 +842,7 @@
result = false;
continue;
}
- if (funnel->FNLnames.data == NULL) {
- funnel->FNLnames.size = length;
- funnel->FNLnames.data = xmalloc(length);
- }
- else if (funnel->FNLnames.size != length) {
- funnel->FNLnames.size = length;
- funnel->FNLnames.data = xrealloc(funnel->FNLnames.data, length);
- }
+ buffer_resize(&funnel->FNLnames, length);
sp->Funnel = funnel - Sites;
}
Index: innd/site.c
===================================================================
--- innd/site.c (revision 9981)
+++ innd/site.c (working copy)
@@ -447,11 +447,11 @@
buffer_append(bp, HDR(HDR__MESSAGE_ID), HDR_LEN(HDR__MESSAGE_ID));
break;
case FEED_FNLNAMES:
- if (sp->FNLnames.data) {
+ if (sp->FNLnames.left != 0) {
/* Funnel; write names of our sites that got it. */
if (Dirty)
buffer_append(bp, ITEMSEP, strlen(ITEMSEP));
- buffer_append(bp, sp->FNLnames.data, sp->FNLnames.used);
+ buffer_append(bp, sp->FNLnames.data, sp->FNLnames.left);
}
else {
/* Not funnel; write names of all sites that got it. */
@@ -516,7 +516,7 @@
case FTprogram:
/* Set up the argument vector. */
if (sp->FNLwantsnames) {
- i = strlen(sp->Param) + sp->FNLnames.used;
+ i = strlen(sp->Param) + sp->FNLnames.left;
if (i + (sizeof(TOKEN) * 2) + 3 >= sizeof buff) {
syslog(L_ERROR, "%s toolong need %lu for %s",
sp->Name, (unsigned long) (i + (sizeof(TOKEN) * 2) + 3),
@@ -523,12 +523,10 @@
sp->Name);
break;
}
- temp = xmalloc(i + 1);
p = strchr(sp->Param, '*');
*p = '\0';
- strlcpy(temp, sp->Param, i + 1);
- strlcat(temp, sp->FNLnames.data, i + 1);
- strlcat(temp, &p[1], i + 1);
+ xasprintf(&temp, "%s%.*s%s", sp->Param, (int) sp->FNLnames.left,
+ sp->FNLnames.data, &p[1]);
*p = '*';
#pragma GCC diagnostic ignored "-Wformat-nonliteral"
snprintf(buff, sizeof(buff), temp, Data->TokenText);
@@ -1071,6 +1069,8 @@
free(sp->FNLnames.data);
sp->FNLnames.data = NULL;
sp->FNLnames.size = 0;
+ sp->FNLnames.left = 0;
+ sp->FNLnames.used = 0;
}
if (sp->HashFeedList) {
for (hf = sp->HashFeedList; hf; hf = hn) {
--
Russ Allbery ([email protected]) <http://www.eyrie.org/~eagle/>
Please send questions to the list rather than mailing me directly.
<http://www.eyrie.org/~eagle/faqs/questions.html> explains why.
------------------------------
_______________________________________________
inn-workers mailing list
[email protected]
https://lists.isc.org/mailman/listinfo/inn-workers
End of inn-workers Digest, Vol 80, Issue 7
******************************************
