Re: [openssl-dev] [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

[Kaduk, Ben via RT]

On 10/05/2016 09:15 AM, Kaduk, Ben via RT wrote:
> I refactored this stuff a while ago to add a flags field that would
> force the temporary read buffer to be allocated from the secure heap; I
> should really dig it up and clean it up for master.

That's https://github.com/openssl/openssl/pull/1700 , FWIW.

-Ben

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

[Kaduk, Ben via RT]

On 10/05/2016 07:56 AM, Richard Levitte via RT wrote:
> To be noted, there's more in section 2:
>
>Most extant parsers ignore blanks at the ends of lines; blanks at the
>beginnings of lines or in the middle of the base64-encoded data are
>far less compatible.  These observations are codified in Figure 1.
>The most lax parser implementations are not line-oriented at all and
>will accept any mixture of whitespace outside of the encapsulation
>boundaries (see Figure 2).  Such lax parsing may run the risk of
>accepting text that was not intended to be accepted in the first
>place (e.g., because the text was a snippet or sample).
>
> I haven't looked enough in our code recently to remember if we're doing
> "standard" (figure 1) or "strict" (figure 3) parsing... what I hear is a
> request for us to move to "lax" (figure 2) parsing.
>

If I remember correctly, it's somewhere in between.  The core
PEM-parsing code is vintage EAY, and contains some "interesting"
behavior, like going to the end of the line/buffer that was read,
backtracking past any characters with ASCII value less than or equal to
that of , and writing \n\0.  So, it seems like trailing
whitespace would be ignored, but leading whitespace would trip up the
"len == 65" check later on.

I refactored this stuff a while ago to add a flags field that would
force the temporary read buffer to be allocated from the secure heap; I
should really dig it up and clean it up for master.

-Ben

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4612] Appcrash on SSL_CTX_new(SSLv2_server_method()) on windows 7 x64 with OpenSSL-1.0.1t

[Kaduk, Ben via RT]

On 08/03/2016 10:12 AM, Viktor Kolodrevskiy wrote:
> Hi,
>
> If I want to enable ssl2 under windows build, will need to pass
> parameters:
> no-asm enable-ssl2 -DOPENSSL_USE_IPV6=0 VC-WIN32
>
> So if I will need to build openssl under linux, parameters should be:
> no-asm enable-ssl2 -DOPENSSL_USE_IPV6=0
>
> Is that right?
>

You still need to pass an os/compiler argument to Configure for linux,
something like linux-x86_64 or linux-elf, presumably.

If you use config instead of Configure, there is autodetection of which
os/compiler to use, so no os/compiler argument is needed.

-Ben

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4612
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4038] SSLv2 session reuse is broken on the 1.0.2 branch

[Kaduk, Ben via RT]

We are patched locally and don’t really need the patch integrated upstream; I 
mostly just wanted to note the issue in the bugtracker in case someone else ran 
into it.

-Ben

On 6/15/16, 08:09, "Salz, Rich via RT"  wrote:

>So are we still fixing SSLv2 bugs?  Or are they too low on the priority list?
>
>
>-- 
>Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4038
>Please log in as guest with password guest if prompted
>
>




-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4038
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #3723] Patch to add short name "Email" to "emailAddress" object

[Kaduk, Ben via RT]

As a bit of follow-up here, it looks like the behavior changed from
using "Email" as a shortname for this attribute to just using the long
form "emailAddress" in commit  30911232c17f309f947156959fcbbf504c1b66fe
back in 2002.  The commit message there was pretty sparse, "Some more
OID enhancements", and there does not seem to be any particular
justification for that change.

However, we've been through a few release series since then, so perhaps
the current behavior is sufficiently "well-established" that it's not
worth reverting back to the historical behavior.

If that's the case, this ticket should probably be closed; if not, we
should be able to come up with a patch that applies cleanly to current
master.

-Ben


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=3723
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4528] Bugfix for linux-armv4 build

[Kaduk, Ben via RT]

On 05/02/2016 10:56 AM, Florent Gluck via RT wrote:
> Hi,
>
> When compiling for linux-armv4 there is a bug in the master branch, 
> version d244dd559d0e6e594e4a0f911e49509e8a7b158b, there is a missing 
> backslash in ssl/record/rec_layer_s3.c.
>

Already fixed in commit fbaf30d087a2db2b4e22279e819d481fca21ac5c

-Ben


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4528
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4489] PATCH: fix Windows deprecated strdup in crypto\conf\conf_lib.c

[Kaduk, Ben via RT]

On 03/29/2016 08:58 AM, noloa...@gmail.com via RT wrote:
> On Tue, Mar 29, 2016 at 9:53 AM, Salz, Rich via RT  wrote:
>> We use strdup because none of the openssl machinery (error stack, etc) might 
>> be set up yet.
>>
>> The comment a few lines above says this!
> Thanks.
>
> That does not explain why this had not effect on Windows, even after
> including "openssl/e_os.h":
>
> # define strdup _strdup
>
> It cleared the warning at other places, but not conf_lib.c.
>

Did you look at the cc -E output for the file in question to see whether
something was preventing the #define from taking effect?

-Ben


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4489
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4378] Multiple warnings under OpenBSD 5.7/64-bit

[Kaduk, Ben via RT]

On 03/04/2016 08:21 PM, noloa...@gmail.com via RT wrote:
> OpenBSD uses GCC 4.2.1
>

This report would be more useful if it gave some indication of what
version of the openssl source it corresponded to.

-Ben


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4378
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4335] ix 'assignment from incompatible type' warning in OBJ_NAME_new_index()

[Kaduk, Ben via RT]

On 02/22/2016 11:04 AM, David Woodhouse via RT wrote:
> We are using OPENSSL_strcmp() as the cmp_func, where cmp_func takes
> a pair of (void *) pointers, not (char *). Which is fine; we know we're
> going to pass it strings in this case. So explicitly cast it to avoid
> the resulting compiler warning.
> ---
>  crypto/objects/o_names.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/objects/o_names.c b/crypto/objects/o_names.c
> index 0a07379..1f9f10a 100644
> --- a/crypto/objects/o_names.c
> +++ b/crypto/objects/o_names.c
> @@ -83,7 +83,7 @@ int OBJ_NAME_new_index(unsigned long (*hash_func) (const 
> char *),
>  return (0);
>  }
>  name_funcs->hash_func = lh_strhash;
> -name_funcs->cmp_func = OPENSSL_strcmp;
> +name_funcs->cmp_func = (void *)OPENSSL_strcmp;
>  CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE);
>  sk_NAME_FUNCS_push(name_funcs_stack, name_funcs);
>  CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ENABLE);
>

Er, where is the cmp_func that is supposed to take void* pointers?  In
master:crypto/objects/o_names.c I see the name_funcs_st with a member
"int (*cmp_func) (const char *a, const char *b);"

(Also, strict C forbids inter-casting between function pointers and
void*, so a cast of (int (*)(void *,void *)) would seem more appropriate
for the stated goal.)

-Ben


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4335
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4256] CA.pl usage() does not mention -signcert

[Kaduk, Ben via RT]

Part of the patch submitted to RT #844 includes a patch to the usage
message of CA.pl.  Although the functionality itself of CA.pl was
rewritten for 1.1 (so that #844 was closed), the usage message remains
incomplete, and Debian continues to apply a local patch to add the usage.

So, as mentioned in the closing message of #844, this is a new ticket
for this lingering issue.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4234] disable EGD by default

[Kaduk, Ben via RT]

Since we can't remove EGD entirely yet, let's do the next best thing.

https://github.com/openssl/openssl/pull/547 contains a patch to add a
no-egd configure option and disable egd support by default.

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4230] apps/speed.c does not handle ghash in the no-SIGALRM case

[Kaduk, Ben via RT]

It looks like in the #ifndef SIGALRM case, c[D_GHASH][0] is set, but not
c[D_GHASH][i] for larger i.

(Where is the no-SIGALRM code used?)

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4227] openssl rand 10000000000 does not produce 10000000000 random bytes

[Kaduk, Ben via RT]

On 01/11/2016 06:01 PM, Salz, Rich via RT wrote:
>> I am a bit worried when I see C-beginner mistakes like this in a security 
>> suite:
>> When using sscanf on data you have not produced yourself, you should
>> always assume they will be bigger that your largest buffer/variable and deal
>> correctly with that.
> That's a bit of an exaggeration here.  It's not network data coming in from 
> somewhere else, it's a number typed on the command line in a local program.
>

There's also the part where asking 'openssl rand' for gigabytes of data
is not necessarily a good idea -- I believe in the default configuration
on unix, it ends up reading 32 bytes from /dev/random and using that to
seed EAY's md_rand.c scheme, which is not exactly a state-of-the-art
CSPRNG these days...


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4183] No SSL_CIPHER_description() for ChaCha20/Poly1305

[Kaduk, Ben via RT]

It looks like SSL_CIPHER_description() will return a bunch of "unknown"s
for the recently added chacha20/poly1305 ciphersuites.

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4155] In function int_thread_del_item, when hash == int_thread_hash, one is passed to free and the other is used in a comparison

[Kaduk, Ben via RT]

On 11/24/2015 05:06 AM, Pascal Cuoq via RT wrote:
> This issue is similar in nature to 4151 
> (http://www.mail-archive.com/openssl-dev@openssl.org/msg40950.html ): it is 
> about a dangling pointer being used, but not used for dereferencing, so it's 
> not a memory error. The dangling pointer is used in a comparison.
>
> The function int_thread_del_item can reach the point were it calls 
> “lh_ERR_STATE_free(int_thread_hash);” with hash == int_thread_hash. That 
> attached patch prints a message like “hash == int_thread_hash == 0xb2a6d0” 
> when this happens. Just after the call to lh_ERR_STATE_free, both hash and 
> int_thread_hash contain dangling pointers. The variable int_thread_hash is 
> immediately set to NULL.
>
> The problem that I am reporting is that just afterwards,  is passed to 
> the function int_thread_release:
>
> https://github.com/openssl/openssl/blob/079a1a9014b89661f0a612a5a9724ad9c77f21a3/crypto/err/err.c#L412
>
>

Note that per J.2 of C99 (n1256.pdf page 503, document-internal
numbering), using the value of a pointer that refers to space
deallocated by a call to free is undefined behavior, even for just
comparison.  (This is covered in passing at
http://web.torek.net/torek/c/numbers2.html)

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4116] [PATCH] Reimplement non-asm OPENSSL_cleanse()

[Kaduk, Ben via RT]

On 11/11/2015 07:06 AM, Kurt Roeckx via RT wrote:
> On Wed, Nov 11, 2015 at 12:37:56PM +, Alessandro Ghedini via RT wrote:
>> On Wed, Nov 11, 2015 at 11:52:56AM +, Kurt Roeckx via RT wrote:
>>> On Wed, Nov 11, 2015 at 11:16:56AM +, Alessandro Ghedini via RT wrote:
 Also, FTR, apparently SecureZeroMemory() doesn't work on the mingw version 
 that
 Travis installs, so that's currently failing to build.
>>> SecureZeroMemory() (and memset_s()) must be supported by the
>>> compiler.  I don't think any compiler other that Microsoft's one
>>> going to support SecureZeroMemory().
>> More recent versions of mingw do support SecureZeroMemory() so that's not the
>> problem, it just so happens that the one installed by Travis is quite 
>> ancient.
> As far as I know mingw uses gcc so how is it able to guarantee
> that SecureZeroMemory() works as intended?

SecureZeroMemory is provided by the Windows system headers; its contract
indicates that it will not be optimized out by compilers (it seems
perform the writes through a volatile char*).

> That's also valid for my implementation IMO. The whole point of "volatile" is
> to prevent the compiler from trying to guess the content of the variable and 
> do
> weird things with it.
>>> As far as I understand volatile will not prevent the compiler from
>>> removing the call.  Volatile will only disable some optimizations,
>>> but it can still see that whatever is written isn't used anymore and
>>> so can be removed.
>> No, when using the volatile pointer the compiler doesn't know if the target
>> function is reentrant or not (because it doesn't know what the target 
>> function
>> actually is) so removing the call is not safe, even by -O3 standards (e.g. 
>> that
>> function might change a global variable or write to a file). Again, that's 
>> the
>> whole point of "volatile".
> So you're saying they assign the function that should be called
> to a volatile pointer, so the compiler can't be sure which
> function is called forcing it to read that volatile pointer all
> the time?
>
> I'm not sure the compiler isn't going to optimize that if it can
> tell nothing else is ever going to write that pointer.  I think
> it's reasonable for it to assume that hardware isn't going to
> change that pointer.
>
The contract of volatile means that the compiler is not permitted to
assume that nothing else is going to write to that variable -- the
compiler doesn't get to make the assumption about hardware not writing
to the variable, even if we humans can.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4125] Got Error

[Kaduk, Ben via RT]

On 11/08/2015 05:37 AM, tamil.sel...@winfoware.com via RT wrote:
> Hi,
> I m getting this error :  if you can solve this problem please reply to this 
> mail.
>   "_BIO_new_mem_buf", referenced from:
>   "_PEM_read_bio_RSA_PUBKEY", referenced from:
>   "_RSA_size", referenced from:
>   "_RSA_public_encrypt", referenced from:
> ld: warning: ld: warning: ignoring file 
> /Users/dvios-mac/Documents/Project/Collections/PaymentIntegation/openssl-1.0.1i-MacOSX/lib/libssl.a,
>  missing required architecture arm64 in file 
> /Users/dvios-mac/Documents/Project/Collections/PaymentIntegation/openssl-1.0.1i-MacOSX/lib/libssl.a
>  (2 slices)ignoring file 
> /Users/dvios-mac/Documents/Project/Collections/PaymentIntegation/openssl-1.0.1i-MacOSX/lib/libcrypto.a,
>  missing required architecture arm64 in file 
> /Users/dvios-mac/Documents/Project/Collections/PaymentIntegation/openssl-1.0.1i-MacOSX/lib/libcrypto.a
>  (2 slices)
>

You probably should show more of the build log, but the "missing
required architecture arm64" is a big warning sign.  Are you trying to
build for arm64?  What measures did you take to do so?

-Ben Kaduk


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4123] Query regarding dummy variable inside crypto

[Kaduk, Ben via RT]

On 11/08/2015 05:37 AM, tosif tamboli via RT wrote:
> Hi ,
> I am compiling crypto in openssl for vxWorks version 5.4
> with 5.5.1 it compiles well
> But with 5.4 it gives error for below files
> bn_depr.c
> ccppc: Internal compiler error: program cc1 got fatal signal 6
>
> When checked it gives error for location
> static void *dummy=
> When I comment it then compiles well. but not getting why it's required if
> it's not being used anywhere.
>
> Can you please provide your inputs for above?
>

It is present so that when OPENSSL_NO_DEPRECATED is set, pedantic
compilers do not warn about an empty compilation unit.  However, it does
not conform to the current best practice for such code.  The attached
patch should help.

-Ben

commit 51aafdce95f998190f91581fa5abf149edcb9c9b
Author: Benjamin Kaduk 
Date:   Mon Nov 9 11:04:35 2015 -0600

Move dummy variable under appropriate conditional

static void *dummy = 
is needed only when OPENSSL_NO_DEPRECATED and PEDANTIC are both set.

diff --git a/crypto/bn/bn_depr.c b/crypto/bn/bn_depr.c
index 34895f5..5932fdd 100644
--- a/crypto/bn/bn_depr.c
+++ b/crypto/bn/bn_depr.c
@@ -64,8 +64,6 @@
 #include "bn_lcl.h"
 #include 
 
-static void *dummy = 
-
 #ifndef OPENSSL_NO_DEPRECATED
 BIGNUM *BN_generate_prime(BIGNUM *ret, int bits, int safe,
   const BIGNUM *add, const BIGNUM *rem,
@@ -112,4 +110,8 @@ int BN_is_prime_fasttest(const BIGNUM *a, int checks,
 return BN_is_prime_fasttest_ex(a, checks, ctx_passed,
do_trial_division, );
 }
+#else /* OPENSSL_NO_DEPRECATED */
+# ifdef PEDANTIC
+static void *dummy = 
+# endif
 #endif
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken

[Kaduk, Ben via RT]

On 11/09/2015 08:41 AM, Hubert Kario via RT wrote:
> Looks like rekeying is on the table again for TLSv1.3:
> https://www.ietf.org/mail-archive/web/tls/current/msg18295.html

The proposed rekeying mechanism is not looking like renegotiation...

> so I'd say this issue should get fixed or at least workarounded (allowed 
> in case the identity doesn't change)
>

...so it's not at all clear that it bears any relevance to this ticket.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Kaduk, Ben via RT]

On 10/22/2015 01:02 PM, stefan.n...@t-online.de via RT wrote:
> Hi,
>
> Wouldn't
>   if ( UINTPTR_MAX - (uintptr_t) buffer < len)
> be closer to the intention of the original check?
> Or is this undefined behaviour as well and I
> stupidly missed that fact?
>

That appears to be defined behavior, but the intention of the original
check is not particularly well-specified.  The committed version should
be sufficient; there does not seem to be a reason to change it again.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Kaduk, Ben via RT]

On 10/16/2015 11:50 AM, Matt Caswell via RT wrote:
>
> On 16/10/15 17:32, Viktor Dukhovni wrote:
>> My take is that we should generally stay clear of relying on any
>> remotely sensible outcome for undefined behaviour.  If this thread
>> is about such a situation, then we may have to code around the
>> issue.
>>
> We are *not* relying on that. Or at least we are only to the extent that
> we are talking about a scenario where something has gone wrong already
> and undefined behaviour is inevitable. We are hoping that our undefined
> behaviour is likely to be slightly less bad than the undefined behaviour
> that we would get otherwise. We cannot know that it will be...but in
> reality there is a reasonable chance that it will.
>
> In a well-behaved program there is no undefined behaviour. The "buf +
> len < buf" check will always evaluate to false, so in that sense is
> useless but it *is* well defined.
>
> In a non well-behaved program if we remove the check then undefined
> behaviour is almost certainly inevitable. Who don't know what it will do
> (it could do anything), but there is a reasonable chance that it could
> result in the disclosure or use of memory it shouldn't be touching. A
> CVE is quite a possible result of such undefined behaviour.
>
> In a non well-behaved program if we keep the check then we still have
> undefined behaviour. The check could do what we kind of expect that it
> might, it might do nothing at all, or it could do something entirely
> different. But without the check undefined behaviour is inevitable
> anyway, so in that sense we are no better off one way or the other. In
> reality however we have a reasonable hope that the check will do
> something like what we hope it might, in which case we will prevent a
> possible security issue.

I think we can have a check in the function that blocks (most of) the
cases we are worried about encountering undefined behavior for, without
actually involving undefined behavior, which at least resembles the best
of both worlds.

Does anyone object to changing the sanity check to be comparing len
against (size_1)-1?  Alternately, checking the range (size_t)-255 to
(size_t)-1?

My personal preference would be to make the function return void and
have this sanity check be an assert() or explicit abort(), but I would
not object to the above given your preference to retain a sanity check.

> That said the packettest test where we deliberately use -1 for a len, is
> actually deliberately relying on undefined behaviour so perhaps should
> be changed. It may also be reasonable to add an additional max length check.
>

It would not rely on undefined behavior with my proposal above.  Of
course, a max length check would supersede my proposal; however, I think
that the PACKET structure may well eventually be used for processing
things other than TLS wire protocol packets, so I would suggest a limit
at least somewhat higher than 2^14+2048.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Kaduk, Ben via RT]

On 10/16/2015 03:32 AM, Matt Caswell via RT wrote:
>
> On 15/10/15 20:53, Alexander Cherepanov via RT wrote:
>> What was not entirely clear from the original bug report is that, while 
>> the check is not compiled away, it's compiled into something completely 
>> different from what is written in the source. Specifically, the check 
>> "buf + len < buf" is optimized into "len >> 63" on 64-bit platform, i.e. 
>> "(ssize_t)len < 0" or "len > SIZE_MAX / 2". This is not a check for 
>> overflow at all, it doesn't even depend on the value of "buf".
>>
>> If this is what was intended then it's better to write it explicitly. If 
>> this is not what was intended then some other approach is required.
> I'd say that is an instance of the compiler knowing better than us how
> big |len| would have to be in order to trigger an overflow. Those rules
> are going to be platform specific so we should not attempt to second
> guess them, but instead let the optimiser do its job.
>

I hope I am not dragging this thread on too long, but with all due
respect, we are not asking the compiler/optimizer to detect overflow --
we are asking the compiler to instantiate undefined behavior in a way
that is convenient for us.  This will only happen by chance, as a side
effect of some other decisions made by the compiler authors, in the
present state of compiler development.

-Ben

P.S. If you haven't encountered it yet,
http://blog.regehr.org/archives/213 et. seq. make for fun reading.


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Kaduk, Ben via RT]

On 10/15/2015 07:41 AM, Matt Caswell via RT wrote:
>
> In summary my opinion is:
> - I believe the sanity check does have some value in guarding against
> programmer error
> - If it were to be compiled away this does not have a detrimental impact
> on security (it just increases the likelihood of a crash in the event of
> a programmer error)

Strictly speaking, it is not a matter of "is the check left as-is" vs.
"is the check compiled away".  C's undefined behavior rules are pretty
open-ended, and the compiler is free to generate code such that, if
inputs that would trigger that check were supplied, does absolutely
anything at all.  Absolutely anything at all means just that; it does
not need to be limited to the local scope and could include exiting from
the program or also reading from /etc/ssh/ssh_host_rsa_key and sending
it over the network.  Now, the compiler is unlikely to do something
"interesting" like that, since it would be at odds with the compiler's
goal of producing fast code, but relying on that does not exactly make
me comfortable.

(N.B. this is not the common case of signed integer overflow that's easy
to reason about; pointer arithmetic has its own rules for undefined
behavior that get invoked when the resulting pointer would not point to
inside (or one past) the same array object that the starting pointer
pointed inside.  This happens in many, many, many more cases than the
current check would catch.  Section 6.5.6 of n1256.pdf covers this topic.)

-Ben

> - There could be a good argument for adding an additional maximum length
> check
> - I do not believe the function should be made void
>


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Kaduk, Ben via RT]

On 10/15/2015 05:44 AM, Emilia Käsper via RT wrote:
> Given OpenSSL's eternal type confusion, this check is meant to trap callers
> that get an error return (typically -1) from some API returning signed values
>
Hmm, do we have a sense for how typically "typically" is?  Maybe just
adding a check for (len == (size_t)-1) is the right thing to do.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4081] crypto/evp/e_dsa.c is orphaned

[Kaduk, Ben via RT]

crypto/evp/e_dsa.c contains only a single static struct variable, and
the file appears unreferenced from anywhere else in the tree.

It should be safe to remove.

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4079] syntax error with EVP_CHECK_DES_KEY

[Kaduk, Ben via RT]

Code inspection of crypto/evp/e_des3.c reveals that !! was used instead
of || (and then subsequently reformatted by a script):

272 # ifdef EVP_CHECK_DES_KEY
273 if (DES_set_key_checked([0], >ks1)
274 ! !DES_set_key_checked([1], >ks2))
275 return 0;

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4026] patches to eliminate some warnings from clang

[Kaduk, Ben via RT]

Updated patch series to address a couple of comments from Ben (made on
now-orphaned github commits).
Still available on github at a rebased
https://github.com/kaduk/openssl/commits/warning-cleanup .

-Ben

>From 4b8104dc7fc3450e43edc1a4fd52ece2ed35929a Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk 
Date: Thu, 6 Aug 2015 13:38:25 -0500
Subject: [PATCH 1/7] Remove some dead code

A duplicate break is not needed inside the preprocessor conditional.

There is no need to assign to ret after an infinite loop with no break
statements.

EXIT() will not return (and even if it did, there's no harm in falling
off the end of main()).  However, EVP_MD_CTX_cleanup() is probably worth
doing before EXIT(), so move it up.
---
 apps/s_client.c | 2 --
 test/sha1test.c | 3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/apps/s_client.c b/apps/s_client.c
index d76f921..9fc8c8c 100644
--- a/apps/s_client.c
+++ b/apps/s_client.c
@@ -830,7 +830,6 @@ int s_client_main(int argc, char **argv)
 BIO_printf(bio_err, "Error getting client auth engine\n");
 goto opthelp;
 }
-break;
 #endif
 break;
 case OPT_RAND:
@@ -2002,7 +2001,6 @@ int s_client_main(int argc, char **argv)
 }
 }
 
-ret = 0;
  shut:
 if (in_init)
 print_stuff(bio_c_out, con, full_log);
diff --git a/test/sha1test.c b/test/sha1test.c
index cc3633d..2bb65e8 100644
--- a/test/sha1test.c
+++ b/test/sha1test.c
@@ -136,9 +136,8 @@ int main(int argc, char *argv[])
 if (err)
 printf("ERROR: %d\n", err);
 #endif
-EXIT(err);
 EVP_MD_CTX_cleanup();
-return (0);
+EXIT(err);
 }
 
 static char *pt(unsigned char *md)
-- 
1.9.1


>From 31d55b8af67c89e1496e32f9f95d45daef171fc9 Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk 
Date: Thu, 6 Aug 2015 13:54:12 -0500
Subject: [PATCH 2/7] constify format string variables to appease
 -Wformat-nonliteral

Only one warning is actually eliminated, though, since (emprically)
only string constants or variables of type char const * const will
avoid the warning.

There are several places where we use a variable to hold one of
a handful of different format strings, which are not trivially
disentangled.  The warning is not especially useful anyway, so
there's no need to try too hard to avoid it.
---
 apps/ocsp.c| 2 +-
 ssl/ssl_ciph.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/apps/ocsp.c b/apps/ocsp.c
index 960b776..21ae581 100644
--- a/apps/ocsp.c
+++ b/apps/ocsp.c
@@ -1167,7 +1167,7 @@ static int do_responder(OCSP_REQUEST **preq, BIO **pcbio, BIO *acbio,
 
 static int send_ocsp_response(BIO *cbio, OCSP_RESPONSE *resp)
 {
-char http_resp[] =
+const char http_resp[] =
 "HTTP/1.0 200 OK\r\nContent-type: application/ocsp-response\r\n"
 "Content-Length: %d\r\n\r\n";
 if (!cbio)
diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c
index 46763d7..793ee47 100644
--- a/ssl/ssl_ciph.c
+++ b/ssl/ssl_ciph.c
@@ -1608,7 +1608,7 @@ char *SSL_CIPHER_description(const SSL_CIPHER *cipher, char *buf, int len)
 const char *ver, *exp_str;
 const char *kx, *au, *enc, *mac;
 unsigned long alg_mkey, alg_auth, alg_enc, alg_mac, alg_ssl;
-static const char *format =
+static const char * const format =
 "%-23s %s Kx=%-8s Au=%-4s Enc=%-9s Mac=%-4s%s\n";
 
 alg_mkey = cipher->algorithm_mkey;
-- 
1.9.1


>From 98746d249f750986cdf21ead522127d2dcebd6a7 Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk 
Date: Thu, 6 Aug 2015 14:53:45 -0500
Subject: [PATCH 3/7] Improve documentation for BIO_set_conn_int_port

BIO_set_conn_int_port is a macro; we can document the type
of its arguments as whatever we want, regardless of what
casts we apply internally.  Make the prototype match what
type we want the input pointer to be and remove a separate
note to that effect.
---
 doc/crypto/BIO_s_connect.pod | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/doc/crypto/BIO_s_connect.pod b/doc/crypto/BIO_s_connect.pod
index 4efd567..0dc2e06 100644
--- a/doc/crypto/BIO_s_connect.pod
+++ b/doc/crypto/BIO_s_connect.pod
@@ -18,7 +18,7 @@ BIO_set_nbio, BIO_do_connect - connect BIO
  long BIO_set_conn_hostname(BIO *b, char *name);
  long BIO_set_conn_port(BIO *b, char *port);
  long BIO_set_conn_ip(BIO *b, char *ip);
- long BIO_set_conn_int_port(BIO *b, char *port);
+ long BIO_set_conn_int_port(BIO *b, int *port);
  char *BIO_get_conn_hostname(BIO *b);
  char *BIO_get_conn_port(BIO *b);
  char *BIO_get_conn_ip(BIO *b, dummy);
@@ -70,8 +70,7 @@ list is http, telnet, socks, https, ssl, ftp, and gopher.
 BIO_set_conn_ip() sets the IP address to B using binary form,
 that is four bytes specifying the IP address in big-endian form.
 
-BIO_set_conn_int_port() sets the port using B. B should
-be of type (int *).
+BIO_set_conn_int_port() sets the port using B.
 
 BIO_get_conn_hostname() returns the hostname of the 

Re: [openssl-dev] [openssl.org #3964] Fix OPENSSL_NO_STDIO build

[Kaduk, Ben via RT]

On 09/30/2015 10:12 AM, Salz, Rich via RT wrote:
>> If things like BIO_new_file() were inline, or macros, then the compiler could
>> *see* that they'd return NULL. And lots of code in the *calling* functions
>> (basically everything but the error path) could be elided from the compiled
>> result...
> Cool, will do that.
>
Making things inline functions or macros requires exposing their
implementation details to consumers of the header in question.  That
seems at odds with the goal of making structures opaque, though I don't
know whether making BIO opaque was one of the targets for that project.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4044] cvsignore files are obsolete

[Kaduk, Ben via RT]

Now that everything's in git, the .cvsignore files seem to serve no
useful purpose, and should be removed.

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4026] patches to eliminate some warnings from clang

[Kaduk, Ben via RT]

I ran a build using clang's -Weverything to activate all warnings (setting
CFLAG in the top-level Makefile and removing all other warning-related
flags, especially -Werror).  With the system clang on my mac, master at
4d04226c2ec7e7f69f6234def63631648e35e828 with the patch from #3984 gives
the following warnings:

  94 [-Wcast-align]
 145 [-Wconversion]
1308 [-Wdocumentation]
  84 [-Wextended-offsetof]
   6 [-Wformat-nonliteral]
 331 [-Wlanguage-extension-token]
 453 [-Wmissing-field-initializers]
   1 [-Wmissing-noreturn]
 237 [-Wmissing-variable-declarations]
7297 [-Wpadded]
 758 [-Wshorten-64-to-32]
1647 [-Wsign-conversion]
  17 [-Wswitch-enum]
   1 [-Wunreachable-code-break]
   5 [-Wunreachable-code]
9422 [-Wunused-macros]
 661 [-Wunused-parameter]

Some of these warnings are not actually useful diagnostics for our use
case and should be ignored (-Wpadded, for example, and
-Wlanguage-extension-token complaining about the use of inline assembly),
so we should not attempt to go for 0 warnings across the board.  But some
of them are helpful to point to places where the code could be improved.


Applying the attached patch series (from git format-patch --stdout) brings
things down to


94 [-Wcast-align]
 145 [-Wconversion]
  84 [-Wextended-offsetof]
   4 [-Wformat-nonliteral]
 331 [-Wlanguage-extension-token]
 453 [-Wmissing-field-initializers]
   1 [-Wmissing-noreturn]
  82 [-Wmissing-variable-declarations]
7297 [-Wpadded]
 759 [-Wshorten-64-to-32]
1645 [-Wsign-conversion]
  17 [-Wswitch-enum]
   3 [-Wunreachable-code]
9422 [-Wunused-macros]
 661 [-Wunused-parameter]

Or, in diff form

3d2
< 1308 [-Wdocumentation]
5c4
<6 [-Wformat-nonliteral]
---
>4 [-Wformat-nonliteral]
9c8
<  237 [-Wmissing-variable-declarations]
---
>   82 [-Wmissing-variable-declarations]
11,12c10,11
<  758 [-Wshorten-64-to-32]
< 1647 [-Wsign-conversion]
---
>  759 [-Wshorten-64-to-32]
> 1645 [-Wsign-conversion]
14,15c13
<1 [-Wunreachable-code-break]
<5 [-Wunreachable-code]
---
>3 [-Wunreachable-code]



The attached patches can also be found at
https://github.com/kaduk/openssl/commits/warning-cleanup .  I did not make
a pull request yet since the commits are broken out for ease of describing
the included changes; they could very well be consolidated into a smaller
number of commits into the main repo.


I was a little uncertain about marking the secure_mem variable in
sec_mem.c as static, since it seemed like the intent was to export that
information; on the other hand, the variable was not declared in any
header, which seems to make it not part of any public API.  From the
comments on pull request 381, it sounds like Rich wants to add an accessor
function, in which case making the actual variable static should be fine.

-Ben Kaduk




warning-cleanup.patch
Description: Binary data
___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4026] patches to eliminate some warnings from clang

[Kaduk, Ben via RT]

Ben's work on df2ee0e27d2db02660c1d15fe6a3e38be9df0a60 made a lot of this stuff 
redundant; I rebased the branch at 
https://github.com/kaduk/openssl/commits/warning-cleanup and generated a new 
warning-cleanup1.patch, attached.  Of note: the move of x509v3/ext_dat.h to 
crypto/include/internal is gone, since it would now just be a lot of churn only 
to remove one ".." path in an include statement.  I also dropped the 
x509_object_cmp() change -- as much as I would like to uncomment the abort(), I 
haven't done an analysis to convince myself that it would not be introducing a 
remote DoS vector.

-Ben

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4038] SSLv2 session reuse is broken on the 1.0.2 branch

[Kaduk, Ben via RT]

SSLv2 support has been removed from master, but is still present in 1.0.2.

Adding a range check in ssl_get_prev_session() broke the SSLv2 codepath
because it supplied NULL as the 'limit' parameter that had not
previously been used for SSLv2 (or v3), so the fix is just to supply a
non-NULL limit.

Patch at https://github.com/openssl/openssl/pull/395 .

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4024] minor tweaks to dsa_gen

[Kaduk, Ben via RT]

Some tweaks in response to my review comments on f00a10b89734e84fe80
(after Rich pointed out where I was misreading the diff).  Also available
via https://github.com/kaduk/openssl/commits/dsa (no pull request since
it's pretty minor)

-Ben




dsa.diff
Description: Binary data
___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #3984] [PATCH] Fix clang compiler warning where %ld is used for uint64_t on Mac OS X

[Kaduk, Ben via RT]

It would be nice to get this or some other fix in; I have to go back and
cherry-pick this commit any time I want to build on OS X.

-Ben Kaduk


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #3999] consider removing the sub-component version strings

[Kaduk, Ben via RT]

It seems like this fine-grained level of detail may have been more
suitable for an earlier point in time when the subcomponents were used.
With the advent of shared libraries, there is less motivation to split
things up in that way, and perhaps the decision to have separate version
strings for each of them should be revisited.

Although these symbols are not prototyped in any public header (which in
some sense makes them not a part of the public/stable API), they are
accessible at link time on at least some systems, and one might imagine
they are in use somewhere.  This would give some weight to the argument
that, if these symbols are to be removed at all, it should only be done at
a (major) version boundary, when ABI-breaking changes are permissible.

For what it's worth (i.e., not very much), clang
-Wmissing-variable-declarations (implied by -Weverything) complains about
these version strings, since there is no forward declaration in advance of
the actual declaration (and they are not static), never mind that forward
declarations are not required by the C standard.

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #3996] master does not build with no-ripemd

[Kaduk, Ben via RT]

Please see https://github.com/openssl/openssl/pull/365 ; just a couple of
tweaks to preprocessor conditionals.

-Ben


___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #3972] EVP documentation implicitly recommends the use of single-DES

[Kaduk, Ben via RT]

See https://github.com/openssl/openssl/pull/348

I was looking for something else but then saw this text about normally
supplied by a function such as EVP_des_cbc(); we should not be misleading
our users in such a fashion.

-Ben

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev