Re: [openssl-dev] [openssl.org #4075] Enhancement request: Camellia ECDHE+GCM suites

[Alessandro Ghedini via RT]

On Mon, Feb 08, 2016 at 05:30:52pm +, Nich Ramsey via RT wrote:
> I said I would be willing to help, but got no reply on how best to ramp up
> on developing a stable addition likely to be accepted by the dev team.

FWIW, the necessary code has already been written (by me) for this particular
feature [0]... the problem isn't lack of code here.

Cheers

[0] https://github.com/openssl/openssl/pull/374

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



signature.asc
Description: PGP signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4271] Enhancement Request: Support TCP Fast Open

[Alessandro Ghedini via RT]

On Mon, Jan 25, 2016 at 06:24:55pm +, Sara Dickinson via RT wrote:
> Hi,
> 
> I would like to request that support be added to OpenSSL to enable client 
> applications to make use use of TCP Fast Open 
> (https://tools.ietf.org/html/rfc7413 ) 
> when initiating the TLS handshake on Linux (TCP Fast Open is available in 
> Linux kernel > 4.1). 
> 
> This was discussed in detail on the OpenSSL Users list:
> https://mta.openssl.org/pipermail/openssl-users/2016-January/002835.html 
> 

I took a stab at implementing TFO support for OpenSSL on Linux and OS X at:
https://github.com/ghedo/openssl/commits/fast_open

This only works for the BIO_s_socket() BIO, but could probably be adapted to
BIO_s_connect() as well if needed.

However I'm not particularly happy with the implementation (it's fairly ugly),
and it would probably be easier to implement this on the application side by
overriding the "write" method of whatever BIO is used, instead of trying to
make OpenSSL do it directly.

Cheers

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



signature.asc
Description: PGP signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4253] [PATCH] Build system fixes for GCC

[Alessandro Ghedini via RT]

Hello,

I opened two pull request regarding fixes for builds using GCC:

* Fix versioned GCC detection
https://github.com/openssl/openssl/pull/552

* Support link time optimization with GCC
https://github.com/openssl/openssl/pull/553

Cheers



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

Re: [openssl-dev] [openssl.org #4157] Download Documentation

[Alessandro Ghedini via RT]

Seems to me this can be closed now.

Cheers


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

Re: [openssl-dev] [openssl.org #4159] BUG ::: Null dereference in ssl3_free

[Alessandro Ghedini via RT]

Kurt said this is fixed in git, can be closed I guess.

Cheers


___
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

[Alessandro Ghedini via RT]

Looks like some things are already fixed in master, does this needs any more
actions?

Cheers


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

Re: [openssl-dev] [openssl.org #4219] [typos] DANE related docs

[Alessandro Ghedini via RT]

Seems fixed in master, so this can be closed.

Cheers


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

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

[Alessandro Ghedini via RT]

Looks fixed in master, can probably be closed.

Cheers


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

Re: [openssl-dev] [openssl.org #4140] GITHUB PULL REQUEST: do not load engines twice

[Alessandro Ghedini via RT]

PR merged, can be closed.

Cheers


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

Re: [openssl-dev] [openssl.org #4112] GH458: Fix "primarility" typo

[Alessandro Ghedini via RT]

PR merged, can be closed.

Cheers


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

Re: [openssl-dev] [openssl.org #4222] Wrong definition of the macro SSL_set1_sigalgs in ssl.h (PR #519)

[Alessandro Ghedini via RT]

PR merged, can be closed now.

Cheers


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

Re: [openssl-dev] [openssl.org #4174] Support the TLS Feature (aka Must Staple) X.509v3 extension (RFC7633)

[Alessandro Ghedini via RT]

PR merged, can be closed.

Cheers


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

Re: [openssl-dev] [openssl.org #4115] [PATCH] Remove remaining FIPS code

[Alessandro Ghedini via RT]

This has been (partially) fixed, so it can probably be closed.


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

Re: [openssl-dev] [openssl.org #4054] [BUG] engine-provided ciphers are unavailable for command-line utility

[Alessandro Ghedini via RT]

Seems that this works in master, so it can probably be closed.

Cheers


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

Re: [openssl-dev] [openssl.org #4239] [PATCH] fixing wildcard matching on punycode domains

[Alessandro Ghedini via RT]

On Fri, Jan 15, 2016 at 06:08:38pm +, Viktor Dukhovni via RT wrote:
> 
> > On Jan 15, 2016, at 10:32 AM, Zi Lin via RT  wrote:
> > 
> > 
> 
> Yes, this will get fixed.  Thanks.

Patches merged, can be closed now.

Cheers


___
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()

[Alessandro Ghedini via RT]

(sorry for the delay, but I've been travelling and moving)

On Sat, Oct 31, 2015 at 11:01:22pm +, Brian Smith via RT wrote:
> On Sat, Oct 31, 2015 at 11:50 AM, Alessandro Ghedini via RT <r...@openssl.org>
> The point is to let the person building OPENSSL say "I want the build to
> fail if there isn't a secure way to zero memory, because I'm expecting
> there to always be one in my configuration." Alternatively, there could be
> an OPENSSL_USE_MEMSET_S flag that says "always use memset_s and never
> anything else."

So, I added memset_s support guarded by the OPENSSL_USE_MEMSET_S ifdef, but it
needs to be specified manually during configuration for now. Ideally it should
be added to the configration of the platforms that support memset_s. I also
added support for explicit_bzero() on OpenBSD. None of this has been tested
though.

However, the thing about OpenSSL's build system is that the asm implementations
will *always* take priority, so you'll also need to specify "no-asm" or this
whole thing won't be built. This is another reason for dropping the assembly
implementations of OPENSSL_cleanse btw.

Also, FTR, apparently SecureZeroMemory() doesn't work on the mingw version that
Travis installs, so that's currently failing to build.

> > > The primary purpose of the assembly language implementations is to
> > reduce the
> > > possibility that the C compiler will do the weird things that C
> > compilers love
> > > to do.
> >
> > According to the changelog and git log, the primary purpose of the asm
> > implementations was to improve performance (see commit b2dba9b). Using the
> > volatile pointer implementation would IMO make these optimizations useless,
> > hence the proposal to drop them and make things simpler.
> 
> 
> Sorry. Instead of "primary purpose" (which implies intent), I should have
> said 'primary advantage".  An assembly language implementation is more
> likely to work than a C implementation because the C compiler generally
> won't analyze externally-assembled code

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. In any case we still need to have support for the no-asm 
case where neither memset_s or SecureZeroMemory are present. And as explained
above, having the asm functions prevents people from actually using the safer
alternatives so I still think it'd be better to just drop them.

In order to avoid having builds that silently optimize away OPENSSL_cleanse
it'd be nice to have a test case to check for that. OpenBSD/LibreSSL have
somthing along those lines [0], but it doesn't seem to work as expected (well,
at least not with OpenSSL), so alternative ideas are welcome.

In any case, I added another commit to my PR that removes the asm functions,
but I left it as a separate commit so it can be easily dropped.

Cheers

[0] 
https://github.com/libressl-portable/openbsd/blob/master/src/regress/lib/libc/explicit_bzero/explicit_bzero.c


___
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()

[Alessandro Ghedini via RT]

On Wed, Nov 11, 2015 at 01:06:54PM +, Kurt Roeckx via RT wrote:
> On Wed, Nov 11, 2015 at 12:37:56PM +0000, 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:
> > > > In order to avoid having builds that silently optimize away 
> > > > OPENSSL_cleanse
> > > > it'd be nice to have a test case to check for that. OpenBSD/LibreSSL 
> > > > have
> > > > somthing along those lines [0], but it doesn't seem to work as expected 
> > > > (well,
> > > > at least not with OpenSSL), so alternative ideas are welcome.
> > > 
> > > As you point out, the compiler can be smart enough to optimize the
> > > call away.  You should at least run that test uing LTO.
> > 
> > Well, yeah, I've been doing my manual tests with LTO all along. My point was
> > that on some platform/compiler/configuration, whatever implementation of
> > OPENSSL_cleanse is used, it could still be optimized away. So it would be
> > useful to have a test to detect that automatically when "make test" is run.
> 
> Even the assembler version gets optimized away?

Sorry, I didn't explain myself very well. What I tried was implement an
obviously unsafe OPENSSL_cleanse (using just memset()) and then check if the
LibreSSL test correctly caught the fact that the call was optimized away. The
test worked in most scenarios, except when building with LTO. So it was only
the test that didn't behave correctly.

Cheers


___
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()

[Alessandro Ghedini via RT]

On Wed, Nov 11, 2015 at 11:52:56AM +, Kurt Roeckx via RT wrote:
> On Wed, Nov 11, 2015 at 11:16:56AM +0000, Alessandro Ghedini via RT wrote:
> > 
> > I also added support for explicit_bzero() on OpenBSD.
> 
> An explicit_bzero() call is no better than whatever
> OPENSSL_cleanse() does, because it has exactly the same problems.
> So I don't think this is useful to do.

Well, yeah, but OpenBSD sort of guarantees that explicit_bzero() is safe. In
any case removing that is ok by me.

> > 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.

> > 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".

> > In order to avoid having builds that silently optimize away OPENSSL_cleanse
> > it'd be nice to have a test case to check for that. OpenBSD/LibreSSL have
> > somthing along those lines [0], but it doesn't seem to work as expected 
> > (well,
> > at least not with OpenSSL), so alternative ideas are welcome.
> 
> As you point out, the compiler can be smart enough to optimize the
> call away.  You should at least run that test uing LTO.

Well, yeah, I've been doing my manual tests with LTO all along. My point was
that on some platform/compiler/configuration, whatever implementation of
OPENSSL_cleanse is used, it could still be optimized away. So it would be
useful to have a test to detect that automatically when "make test" is run.

However the test provided by OpenBSD/LibreSSL doesn't work, that is, with some
configurations (e.g. when LTO is used) it doesn't actually detect that the call
has been optimized away, so a smarter test is required, but I have no idea what
that might look like.

Cheers



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

[openssl-dev] [openssl.org #4113] [PATCH] Cleanup and update README

[Alessandro Ghedini via RT]

Hi,

the current README in master contains a lot of outdated information and some
weird wording, so I prepared a patch to fix it.

See the following GitHub pull request:
https://github.com/openssl/openssl/pull/457

Cheers

___
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 #4114] Continuous integration for Windows

[Alessandro Ghedini via RT]

Hi,

the current Travis CI setup lacks support for proper Windows support, so I
prepared a patch to add configuration for the AppVeyor service [0] which
provides continuous integration on Windows.

See the following GitHub pull request:
https://github.com/openssl/openssl/pull/456

Cheers

[0] http://www.appveyor.com/

___
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 #4116] [PATCH] Reimplement non-asm OPENSSL_cleanse()

[Alessandro Ghedini via RT]

Hi,

the current platform-generic implementation of OPENSSL_cleanse() is very weird
and IMO overly complex (its initial intent was to cleanse with values other
than 0, but AFAICT none of the asm implementations do it), so I reimplemented
it in a simpler way.

I was also wondering whether it would make sense to just drop the asm
implementations. Does the speed-up justify the added complexity?

See the following GitHub pull request:
https://github.com/openssl/openssl/pull/455

Cheers

___
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 #4115] [PATCH] Remove remaining FIPS code

[Alessandro Ghedini via RT]

Hi,

I don't know what your intentions are with FIPS support in master, but after
the removal of most if the fips/ code, several bits and pieces of now broken
code have remained in the codebase. IMO it'd be better to just remove it for
now.

See the following GitHub pull request:
https://github.com/openssl/openssl/pull/449

Cheers

___
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 #4117] [PATCH] Remove useless locking code

[Alessandro Ghedini via RT]

Hi,

in commit 070c233 I didn't notice that the CRYPTO_w_lock()/CRYPTO_w_unlock()
calls are now useless, so I made a patch to fix that.

See the following GitHub pull request:
https://github.com/openssl/openssl/pull/454

Cheers

___
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 #4116] [PATCH] Reimplement non-asm OPENSSL_cleanse()

[Alessandro Ghedini via RT]

On Sat, Oct 31, 2015 at 07:59:03PM +, Brian Smith via RT wrote:
> Alessandro Ghedini via RT <r...@openssl.org> wrote:
> 
> > I was also wondering whether it would make sense to just drop the asm
> > implementations. Does the speed-up justify the added complexity?
> >
> 
> IMO, it should work like this:
> * memset_s when memset_s is available.

Given the current OpenSSL build system, there's no way to detect if memset_s is
available at build time (unless it's defined as a macro).

In any case memset_s is not available anywhere anyway, so that doesn't really
matter.

> * Otherwise, SecureZeroMemory, when SecureZeroMemory is available.
> * Otherwise, if a flag OPENSSL_REQUIRE_SECURE_ZERO is set, fail.

In 99% of the cases (e.g. Linux with glibc or any *BSD) that would fail, so I
don't see the point in that.

> * Otherwise, use an assembly language implementation, if available.
> * Otherwise, emit a warning and use the C implementation.
> 
> Note in particular that the C compiler is allowed to completely defeat the
> purpose of the function unless SecureZeroMemory or memset_s is used, even
> if you use "volatile" or other tricks.

I don't think that is true (regarding the volatile pointer). But assuming that
a broken compiler decided to optimize that call away, what's stopping it to
optimize the call to the asm implementation as well? Also, such broken compiler
probably wouldn't know about C11 either, so even if memset_s() was available it
could optimize that as well.

I don't know how compilers are supposed to treat memset_s(), but if I define a
memset_s() function which just calls memset() internally, GCC (v5.2.1 20151028)
optimizes that away just as it does with plain memset(), so libc
implementations would probably need to adopt "tricks" to avoid the optimization
as well.

FWIW OpenSSH implements the portable explicit_bzero() using the volatile
pointer as well (unless memset_s() is detected at build time). On OpenBSD it
just uses OpenBSD's explicit_bzero() which is implemented using memset() and a
weak function pointer. But that (as used in LibreSSL) seems to have problems in
relation to LTO, unless optimizations are specifically disabled:
https://github.com/libressl-portable/openbsd/issues/5

On the other hand BoringSSL uses memset() with an explicit memory barrier
implemented as inline assembly, which is possibly better than the volatile
implementation because GCC (and probably other compilers) can inline the
memset() call. But if OPENSSL_NO_ASM is defined, the barrier is omitted and
memset() is optimized away, so that's potentially dangerous.

The above is just to give an overview on how other projects solved this
problem. Notably none of them uses full assembly implementations like OpenSSL
does.

> The primary purpose of the assembly language implementations is to reduce the
> possibility that the C compiler will do the weird things that C compilers love
> to do.

According to the changelog and git log, the primary purpose of the asm
implementations was to improve performance (see commit b2dba9b). Using the
volatile pointer implementation would IMO make these optimizations useless,
hence the proposal to drop them and make things simpler.

Cheers


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

Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Fri, Oct 09, 2015 at 05:02:47pm +, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 07:57:21pm +0000, Alessandro Ghedini via RT wrote:
> > FYI, I just pushed another patch that does the above (moving the check and
> > sending an alert) which I think is the best option (although, as Hubert 
> > pointed
> > out, sending the decode_error alert is not a MUST). If that's ok with you, I
> > can squash the two commits together and give them a better message, 
> > otherwise
> > just ignore the second patch.
> 
> So, I went ahead and squashed all the commits into one [0] and also added the
> SSLv2 check as well. Can someone please have a look?

Ping? FYI I just rebased my patch at [0] on top of the state machine rewrite
commits in master (in fact I've rebased all my patches on master).

Cheers

[0] https://github.com/openssl/openssl/pull/437


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

[openssl-dev] [openssl.org #4090] [PATCH] Assorted fixes

[Alessandro Ghedini via RT]

Hello,

I've prepared a few patches to fix several minor-ish issues (I though it didn't
make much sense to submit them one by one). See GitHub pull request at:
https://github.com/openssl/openssl/pull/436

The patches are:

- Do not treat 0 return value from BIO_get_fd() as error (fixes RT#4068)
- Replace malloc+strlcpy with strdup
- Fix memory leaks and other mistakes on errors
- Set salt length after the malloc has succeeded
- Fix typos
- Fix references to various RFCs
- Check memory allocation
- Remove useless code (fixes RT#4081)

(some of the issues were found using Clang's static analyzer).

They should be all pretty self-explanatory.

Cheers

___
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 #3712] TLS Renegotiation with Java is broken

[Alessandro Ghedini via RT]

On Mon, Oct 12, 2015 at 01:45:20PM +, Hubert Kario via RT wrote:
> On Friday 09 October 2015 18:05:19 Matt Caswell via RT wrote:
> > On 09/10/15 19:02, Hubert Kario via RT wrote:
> > > And for good measure, I also created a test script that
> > > combines fragmentation with interleaving.
> > 
> > Did you try my patch with it? And if so what happened?
> 
> I'm using interleave-data-102.patch attached to this ticket.
> 
> So, for state-machine-rewrite branch it doesn't apply, there's no 
> ssl/s3_pkt.c file.
> 
> For current 1.0.1 branch, the patch applies, test case results are as 
> follows:
>  * test-openssl-3712.py - pass
>  * test-interleaved-application-data-in-renegotiation.py - pass
>  * test-interleaved-application-data-and-fragmented-handshakes-in-
> renegotiation.py - pass
> 
> For current 1.0.2 branch, the patch applies, tests case results are as 
> follows:
>  * test-openssl-3712.py - pass
>  * test-interleaved-application-data-in-renegotiation.py - pass
>  * test-interleaved-application-data-and-fragmented-handshakes-in-
> renegotiation.py - pass
> 
> for current master the patch doesn't apply, just like with state-
> machine-rewrite there's no ssl/s3_pkt.c file
> 
> Note: the two latter test cases need the s_server run in -www mode, the 
> first test case ignores server response so will work regardless, that 
> may be why Alessandro testing doesn't show the issue as fixed

Ah, yep, with -www it works for me too. Note that on master the file to change
should be ssl/record/ssl3_record.c. However, while the patch applies cleanly to
this file, all the tests fail (even with -www). It seems that the field
in_read_app_data is never true, so the UNEXPECTED_MESSAGE alert is sent.

Cheers


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

Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 07:57:21pm +, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 06:26:27pm +0000, Alessandro Ghedini via RT wrote:
> > On Thu, Oct 08, 2015 at 06:14:00pm +, Alessandro Ghedini via RT wrote:
> > > On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote:
> > > > On Thu, Oct 08, 2015 at 04:12:50pm +, Hubert Kario via RT wrote:
> > > > > The server does not abort connection upon receiving a Client Hello 
> > > > > message with malformed session_id field.
> > > > > 
> > > > > Affects 1.0.1, 1.0.2 and master.
> > > > > 
> > > > > In SSLv3 and all versions of TLS (e.g. RFC 5246), the SessionID is 
> > > > > defined as
> > > > > 
> > > > >   opaque SessionID<0..32>;
> > > > > 
> > > > > that means, that any SessionID longer than 32 bytes creates an 
> > > > > incorrectly formatted Client Hello message, and as such, should be 
> > > > > rejected.
> > > > 
> > > > Looking at the code in master, for non-v2 ClientHello messages the code 
> > > > uses
> > > > the PACKET_get_length_prefixed_1() function to extract the SessionID, 
> > > > however I
> > > > see no way to pass a maximum allowed length to it. I think a new 
> > > > function would
> > > > have to be added to the PACKET_* interface (I can look into this). 
> > > > Haven't
> > > > checked older branches yet.
> > > 
> > > So, it turns out the check was already performed, but this failure didn't 
> > > cause
> > > the session to be aborted (the original PACKET was advanced anyway 
> > > though, even
> > > is the session_id isn't actually extracted), I don't know if this was on
> > > purpose though.
> > 
> > Another thing to consider is that the client already aborts when an invalid
> > session_id is received in the ServerHello and sends the ILLEGAL_PARAMETER 
> > alert.
> > 
> > My patch doesn't actually send any alert so the check should be moved 
> > earlier
> > to allow an alert to be sent, if that is desired.
> 
> FYI, I just pushed another patch that does the above (moving the check and
> sending an alert) which I think is the best option (although, as Hubert 
> pointed
> out, sending the decode_error alert is not a MUST). If that's ok with you, I
> can squash the two commits together and give them a better message, otherwise
> just ignore the second patch.

So, I went ahead and squashed all the commits into one [0] and also added the
SSLv2 check as well. Can someone please have a look?

Anyway, I noticed that the client compares the session_id length against
"sizeof s->session->session_id" (which is SSL_MAX_SSL_SESSION_ID_LENGTH, like I
used in my patch), and also against SSL3_SESSION_ID_SIZE (which is equal to
SSL_MAX_SSL_SESSION_ID_LENGTH). I think this second check is superfluous and
the other one can just use SSL_MAX_SSL_SESSION_ID_LENGTH directly instead of
sizeof (it'd be more obvious).

Cheers

[0] https://github.com/openssl/openssl/pull/437


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

Re: [openssl-dev] [openssl.org #4084] correction to the message i sent earlier...

[Alessandro Ghedini via RT]

This was supposed to be a reply to another message (#4083), but a new report
has been created instead. I think it can be closed.

Cheers


___
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

[Alessandro Ghedini via RT]

On Fri, Oct 09, 2015 at 06:05:19pm +, Matt Caswell via RT wrote:
> 
> 
> On 09/10/15 19:02, Hubert Kario via RT wrote:
> > And for good measure, I also created a test script that
> > combines fragmentation with interleaving.
> 
> Did you try my patch with it? And if so what happened?

I just tried both the test-interleaved-application-data-in-renegotiation.py and
test-interleaved-application-data-and-fragmented-handshakes-in-renegotiation.py
scripts and they both fail with your patch applied. In fact, your patch made
one more test from the first script to fail (3 failures with your patch, only
2 without the patch). On the other hand your patch makes the test in the
test-openssl-3712.py script succeed (which was the original test for thus bug
AFAIU).

I haven't really followed this discussion though, so I'm not sure what the
outcome should be (I'd imagine all tests should pass though).

Cheers


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

Re: [openssl-dev] [openssl.org #4075] Enhancement request: Camellia ECDHE+GCM suites

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 12:47:21AM +, Moonchild via RT wrote:
> Hello people,
> 
> An enhancement request here for OpenSSL to add support for Camellia in GCM
> with ECC key exchange.
> 
> Rationale:
> Camellia has been recognized as a modern and supported cipher by ENISA,
> NESSIE, CRYPTREC, ISO and IETF among others so should be supported
> long-term. OpenSSL currently only supports the (rather expensive) DHE/RSA
> CBC+IV versions of the suite, and should be updated with the ECC and GCM
> modes of operation.
> 
> It's important to have at least one cipher coming from non-US expert bodies
> that is maintained to the same level as AES currently is, and OpenSSL seems
> to be trailing behind in that respect. I would request addition of at least
> the following:
> 
> TLS_ECDHE_ECDSA_WITH_CAMELLIA_128_GCM_SHA256 (0xc086)
> TLS_ECDHE_RSA_WITH_CAMELLIA_128_GCM_SHA256 (0xc08a)
> And possibly their 256-bit counterparts

Patches for this are available at [0], however there has been some resistance
to adding the new TLS cipher suites to OpenSSL (see [1]), so the discussion has
stalled.

> These suites are already supported in e.g. GNUTLS, Botan and PolarSSL, iiuc.
> Firefox will also be adding the GCM versions of Camellia to NSS

Do you have a source for the news above? IIRC Firefox used to support Camellia,
but dropped it in v37 or so.

Cheers

[0] https://github.com/openssl/openssl/pull/374
[1] https://rt.openssl.org/Ticket/Display.html?id=4017


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

Re: [openssl-dev] [openssl.org #4075] Enhancement request: Camellia ECDHE+GCM suites

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 11:39:56am +, Salz, Rich via RT wrote:
> Also, note that the earliest this could happen is for 1.1 (it's a new
> feature), and it's not high on our priority list for that release right now.
> Patches that are regularly rebased against master would help.

I rebase my patches on master regularly when changes that cause merge conflicts
are pushed, so my camellia patches should merge cleanly.

Cheers


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

Re: [openssl-dev] [openssl.org #3982] [PATCH] Fix unhandled error condition in sslv2 client hello parsing

[Alessandro Ghedini via RT]

The GitHub pull request was merged, so this can be closed now.

Cheers


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

Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 04:12:50pm +, Hubert Kario via RT wrote:
> The server does not abort connection upon receiving a Client Hello 
> message with malformed session_id field.
> 
> Affects 1.0.1, 1.0.2 and master.
> 
> In SSLv3 and all versions of TLS (e.g. RFC 5246), the SessionID is 
> defined as
> 
>   opaque SessionID<0..32>;
> 
> that means, that any SessionID longer than 32 bytes creates an 
> incorrectly formatted Client Hello message, and as such, should be 
> rejected.

Looking at the code in master, for non-v2 ClientHello messages the code uses
the PACKET_get_length_prefixed_1() function to extract the SessionID, however I
see no way to pass a maximum allowed length to it. I think a new function would
have to be added to the PACKET_* interface (I can look into this). Haven't
checked older branches yet.

The problem most likely happens with SSLv2 backwards compatible ClientHello as
well, but that seems to be easier to fix... or maybe it's time to just drop
that compatibility code for v1.1?

Cheers


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

Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 06:14:00pm +, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 05:19:06pm +0000, Alessandro Ghedini via RT wrote:
> > On Thu, Oct 08, 2015 at 04:12:50pm +, Hubert Kario via RT wrote:
> > > The server does not abort connection upon receiving a Client Hello 
> > > message with malformed session_id field.
> > > 
> > > Affects 1.0.1, 1.0.2 and master.
> > > 
> > > In SSLv3 and all versions of TLS (e.g. RFC 5246), the SessionID is 
> > > defined as
> > > 
> > >   opaque SessionID<0..32>;
> > > 
> > > that means, that any SessionID longer than 32 bytes creates an 
> > > incorrectly formatted Client Hello message, and as such, should be 
> > > rejected.
> > 
> > Looking at the code in master, for non-v2 ClientHello messages the code uses
> > the PACKET_get_length_prefixed_1() function to extract the SessionID, 
> > however I
> > see no way to pass a maximum allowed length to it. I think a new function 
> > would
> > have to be added to the PACKET_* interface (I can look into this). Haven't
> > checked older branches yet.
> 
> So, it turns out the check was already performed, but this failure didn't 
> cause
> the session to be aborted (the original PACKET was advanced anyway though, 
> even
> is the session_id isn't actually extracted), I don't know if this was on
> purpose though.

Another thing to consider is that the client already aborts when an invalid
session_id is received in the ServerHello and sends the ILLEGAL_PARAMETER alert.

My patch doesn't actually send any alert so the check should be moved earlier
to allow an alert to be sent, if that is desired.

Cheers


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

Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 04:12:50pm +, Hubert Kario via RT wrote:
> > The server does not abort connection upon receiving a Client Hello 
> > message with malformed session_id field.
> > 
> > Affects 1.0.1, 1.0.2 and master.
> > 
> > In SSLv3 and all versions of TLS (e.g. RFC 5246), the SessionID is 
> > defined as
> > 
> >   opaque SessionID<0..32>;
> > 
> > that means, that any SessionID longer than 32 bytes creates an 
> > incorrectly formatted Client Hello message, and as such, should be 
> > rejected.
> 
> Looking at the code in master, for non-v2 ClientHello messages the code uses
> the PACKET_get_length_prefixed_1() function to extract the SessionID, however 
> I
> see no way to pass a maximum allowed length to it. I think a new function 
> would
> have to be added to the PACKET_* interface (I can look into this). Haven't
> checked older branches yet.

So, it turns out the check was already performed, but this failure didn't cause
the session to be aborted (the original PACKET was advanced anyway though, even
is the session_id isn't actually extracted), I don't know if this was on
purpose though.

In any case I wrote a minimal patch that makes the tlsfuzzer test pass (it may
even work for SSLv2 as well):
https://github.com/openssl/openssl/pull/437

Cheers


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

Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 06:26:27pm +, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 06:14:00pm +0000, Alessandro Ghedini via RT wrote:
> > On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote:
> > > On Thu, Oct 08, 2015 at 04:12:50pm +, Hubert Kario via RT wrote:
> > > > The server does not abort connection upon receiving a Client Hello 
> > > > message with malformed session_id field.
> > > > 
> > > > Affects 1.0.1, 1.0.2 and master.
> > > > 
> > > > In SSLv3 and all versions of TLS (e.g. RFC 5246), the SessionID is 
> > > > defined as
> > > > 
> > > >   opaque SessionID<0..32>;
> > > > 
> > > > that means, that any SessionID longer than 32 bytes creates an 
> > > > incorrectly formatted Client Hello message, and as such, should be 
> > > > rejected.
> > > 
> > > Looking at the code in master, for non-v2 ClientHello messages the code 
> > > uses
> > > the PACKET_get_length_prefixed_1() function to extract the SessionID, 
> > > however I
> > > see no way to pass a maximum allowed length to it. I think a new function 
> > > would
> > > have to be added to the PACKET_* interface (I can look into this). Haven't
> > > checked older branches yet.
> > 
> > So, it turns out the check was already performed, but this failure didn't 
> > cause
> > the session to be aborted (the original PACKET was advanced anyway though, 
> > even
> > is the session_id isn't actually extracted), I don't know if this was on
> > purpose though.
> 
> Another thing to consider is that the client already aborts when an invalid
> session_id is received in the ServerHello and sends the ILLEGAL_PARAMETER 
> alert.
> 
> My patch doesn't actually send any alert so the check should be moved earlier
> to allow an alert to be sent, if that is desired.

FYI, I just pushed another patch that does the above (moving the check and
sending an alert) which I think is the best option (although, as Hubert pointed
out, sending the decode_error alert is not a MUST). If that's ok with you, I
can squash the two commits together and give them a better message, otherwise
just ignore the second patch.

Cheers


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

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

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 04:18:53pm +, Kaduk, Ben via RT wrote:
> 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.

This is now fixed in my "Remove useless code" patch at
https://github.com/openssl/openssl/pull/436

Cheers


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

Re: [openssl-dev] [openssl.org #4068] Bug ocsp - bio_get_fd

[Alessandro Ghedini via RT]

On Fri, Oct 02, 2015 at 02:06:12am +, vince technical address via RT wrote:
> Hi,
> 
> Can you tell me why in the source file "ocsp.c" (apps directory), the test
> on the return of the function BIO_get_fd defines 0 as an invalid file
> descriptor?
> 
> if (BIO_get_fd (CBIO, & fd) <= 0)
> 
> should be
> 
> if (BIO_get_fd (CBIO, & fd) <0)

For reference, I opened a GitHub  pull request fixing this at:
https://github.com/openssl/openssl/pull/423

Cheers


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

Re: [openssl-dev] [openssl.org #4069] Malformed Client Hello messages are accepted (custom message padding and length)

[Alessandro Ghedini via RT]

On Fri, Oct 02, 2015 at 11:26:36am +, Hubert Kario via RT wrote:
> Current git checkout of 1.0.1, 1.0.2 and master accept malformed Client
> Hello messages.
> 
> If the client sends a Client Hello message with extensions.length field
> equal to 0, but padded with bytes
> FF01 0001 00
> then the Server Hello will contain the renegotiation_info extension.

Yup, ssl_scan_clienthello_tlsext() extracts the length but then it doesn't do
anything with it.

I wrote a patch [0] that fixes this specific problem in master, but the
tlsfuzzer script has a bunch of other failures. Incidentally, with my patch
applied, the tlsfuzzer test takes a lot less time (like it's seconds faster),
not quite sure if that's good or bad...

Cheers

[0] https://github.com/openssl/openssl/pull/421


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

Re: [openssl-dev] [openssl.org #4069] Malformed Client Hello messages are accepted (custom message padding and length)

[Alessandro Ghedini via RT]

On Fri, Oct 02, 2015 at 11:51:10am +, Alessandro Ghedini via RT wrote:
> On Fri, Oct 02, 2015 at 11:26:36am +, Hubert Kario via RT wrote:
> > Current git checkout of 1.0.1, 1.0.2 and master accept malformed Client
> > Hello messages.
> > 
> > If the client sends a Client Hello message with extensions.length field
> > equal to 0, but padded with bytes
> > FF01 0001 00
> > then the Server Hello will contain the renegotiation_info extension.
> 
> Yup, ssl_scan_clienthello_tlsext() extracts the length but then it doesn't do
> anything with it.
> 
> I wrote a patch [0] that fixes this specific problem in master, but the
> tlsfuzzer script has a bunch of other failures. Incidentally, with my patch
> applied, the tlsfuzzer test takes a lot less time (like it's seconds faster),
> not quite sure if that's good or bad...

I updated my patch as Matt suggested, and now all the failures seem to be gone.

Cheers


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

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

[Alessandro Ghedini via RT]

On Wed, Sep 30, 2015 at 02:01:54am +, Rich Salz via RT wrote:
> We fixed this in a slightly different way. We made BIO_new_file and BIO_s_file
> return an alternate implementation that returns run-time failures. Almost all
> of the OpenSSL code uses the BIO object, so we didn't have to remove that. We
> did #ifdef out any routine that had a "FILE*" param or local variable.

Note that commit 984d6c6 causes mingw shared builds to fail again:
https://travis-ci.org/openssl/openssl/jobs/82855661

The problem seems to be that entries 4991 and 4992 in libeay.num are used twice.

Since this is not the first time this happens, I was thinking that maybe having
a test for this would be useful.

Cheers


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

Re: [openssl-dev] [openssl.org #3986] [PATCH] Implement HKDF algorithm (RFC 5869)

[Alessandro Ghedini via RT]

Just FYI, I updated the GitHub pull request [0] with the following:

- Merged patches into a single commit. This just makes more sense, and it's not
  much more complicated to review.
- Added HKDF_Extract() function to the interface. This is basically equivalent
  to calling HMAC(), but the TLS 1.3 draft started using it explicitly so I
  thought it would make the API more complete.
- Added documentation for all the new functions (see doc/crypto/hkdf.pod).
- Updated util/mkdef.pl to parse hkdf.h as well, and updated util/libeay.num.

Now all the builds on Travis CI run fine (except the ones that were already
failing).

Cheers

[0] https://github.com/openssl/openssl/pull/355


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

Re: [openssl-dev] [openssl.org #4063] Client Hello longer than 2^14 bytes are rejected

[Alessandro Ghedini via RT]

On Fri, Sep 25, 2015 at 02:02:36pm +, Hubert Kario via RT wrote:
> On Friday 25 September 2015 13:55:56 Alessandro Ghedini via RT wrote:
> > On Fri, Sep 25, 2015 at 01:20:12pm +, Hubert Kario via RT wrote:
> > > Current OpenSSL-1.0.1, 1.0.2 as well as state-machine-rewrite
> > > branches reject Client Hello messages bigger than 2^14+4 bytes.
> > 
> > IIRC SSLv3 does place the limit at 2^14 or so bytes, so I think the
> > problem is that OpenSSL only checks for that.
> 
> yes, it does place a limit of 2^14, but only on _records_, not handshake 
> messages that travel in those records
> 
> > I think a proper fix would be to have all the ssl_get_message() calls
> > changed to use the proper "max" parameter depending on the protocol
> > version.
> 
> As far as I can tell, SSLv3, TLS1.0, TLS1.1 and TLS1.2 are exactly the 
> same as in they don't specify any upper size limit for the Handshake 
> protocol messages or Client Hello specifically other than the limits 
> enforced by the length fields themselves.
> 
> Remember, the records are completely independent of messages that travel 
> through them, record layer is just there to multiplex the different 
> protocols that are required for TLS to work (handshake, CCS, application 
> data, etc.)

Right. Some of the handshake messages do have a maximum length though (e.g.
ChangeCipherSpace), so the maximum length check shouldn't be removed (as a
sanity check). In the case of ClientHello, the minimal fix for the problem
then is just a matter of finding the absolute maximum it can hold (which may
as well be whatever the Handshake length field maximum value is).

As a matter of test I changed the ssl_get_message() in ssl3_get_client_hello()
to use 0xFF (uint24 max) as maximum size, and the tlsfuzzer failures went
from 5 to 2, are all the tests supposed to pass? If so, then there's another
problem as well.

Cheers


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

Re: [openssl-dev] [openssl.org #4063] Client Hello longer than 2^14 bytes are rejected

[Alessandro Ghedini via RT]

On Fri, Sep 25, 2015 at 03:02:27pm +, Hubert Kario via RT wrote:
> On Friday 25 September 2015 14:51:17 Alessandro Ghedini via RT wrote:
> > As a matter of test I changed the ssl_get_message() in
> > ssl3_get_client_hello() to use 0xFF (uint24 max) as maximum size,
> 
> it doesn't have in theory, but it does in practice, as extensions can 
> only be 2^16 long, same for cipher suites, and you can't have data 
> trailing the messages, so the actual size is limited to something closer 
> 2^18, so if the client hello parser is correct, it will be limited by it

Yeah, but OpenSSL first tries to "get" the handshake body and its length before
parsing it (this is done by ssl3_get_message()). So the "max" argument is
intended to be used, I imagine, as a sanity check: if the message exceeds that,
then it's obviously broken and an "illegal parameters" alert is sent. This is
done regardless of the message type, so the ClientHello parser has to do this
as well.

This max length check is not exactly smart (e.g. the max size of the SSLv3
ClientHello is very different from that of TLS) and could probably be removed
completely, but I don't really know what the consequences of this would be. So
the best next fix would simply be to provide an approximation of an absolute
maximum length for the ClientHello (or just use 0xFF). I opened a pull
request [0] with just this minimal fix. Anyone is very welcome to propose a
better fix for this though.

How trailing data is then handled is orthogonal to this problem (the length
check doesn't really affect this). It might be that OpenSSL handles it
correctly or not, I don't really know (and having the tlsfuzzer test for this
will be very useful).

Cheers

[0] https://github.com/openssl/openssl/pull/413


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

Re: [openssl-dev] [openssl.org #4063] Client Hello longer than 2^14 bytes are rejected

[Alessandro Ghedini via RT]

On Fri, Sep 25, 2015 at 04:17:33PM +, Matt Caswell via RT wrote:
> 
> 
> On 25/09/15 17:05, Alessandro Ghedini via RT wrote:
> > On Fri, Sep 25, 2015 at 03:02:27pm +, Hubert Kario via RT wrote:
> >> On Friday 25 September 2015 14:51:17 Alessandro Ghedini via RT wrote:
> >>> As a matter of test I changed the ssl_get_message() in
> >>> ssl3_get_client_hello() to use 0xFF (uint24 max) as maximum size,
> >>
> >> it doesn't have in theory, but it does in practice, as extensions can 
> >> only be 2^16 long, same for cipher suites, and you can't have data 
> >> trailing the messages, so the actual size is limited to something closer 
> >> 2^18, so if the client hello parser is correct, it will be limited by it
> > 
> > Yeah, but OpenSSL first tries to "get" the handshake body and its length 
> > before
> > parsing it (this is done by ssl3_get_message()). So the "max" argument is
> > intended to be used, I imagine, as a sanity check: if the message exceeds 
> > that,
> > then it's obviously broken and an "illegal parameters" alert is sent. This 
> > is
> > done regardless of the message type, so the ClientHello parser has to do 
> > this
> > as well.
> > 
> > This max length check is not exactly smart (e.g. the max size of the SSLv3
> > ClientHello is very different from that of TLS) and could probably be 
> > removed
> > completely, but I don't really know what the consequences of this would be. 
> > So
> > the best next fix would simply be to provide an approximation of an absolute
> > maximum length for the ClientHello (or just use 0xFF). I opened a pull
> > request [0] with just this minimal fix. Anyone is very welcome to propose a
> > better fix for this though.
> 
> 0xff = 16777215 or 16Mb
> 
> Allowing a ClientHello as big as this could enable a DoS attack.
> 
> If I did my sums right I make the biggest possible valid ClientHello to
> be 131396.

I updated my patch to use this value now.

> But should we allow something as big as this just because its
> theoretically possible?

As a way of future-proofing OpenSSL (Hubert mentioned a few reasons) or just
to be more standard-compliant I'd say yes, but it's obviously not an urgent
problem.

FWIW I checked a couple of TLS implementations I have around (GnuTLS and s2n),
and AFAICT they don't check for a maximum size at all.

Cheers


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

Re: [openssl-dev] [openssl.org #4063] Client Hello longer than 2^14 bytes are rejected

[Alessandro Ghedini via RT]

On Fri, Sep 25, 2015 at 05:11:39pm +, Hubert Kario via RT wrote:
> On Friday 25 September 2015 16:54:02 Alessandro Ghedini via RT wrote:
> > On Fri, Sep 25, 2015 at 04:17:33PM +, Matt Caswell via RT wrote:
> > > On 25/09/15 17:05, Alessandro Ghedini via RT wrote:
> > > > On Fri, Sep 25, 2015 at 03:02:27pm +, Hubert Kario via RT 
> wrote:
> > > >> On Friday 25 September 2015 14:51:17 Alessandro Ghedini via RT 
> wrote:
> > > >>> As a matter of test I changed the ssl_get_message() in
> > > >>> ssl3_get_client_hello() to use 0xFF (uint24 max) as maximum
> > > >>> size,
> > > >> 
> > > >> it doesn't have in theory, but it does in practice, as extensions
> > > >> can
> > > >> only be 2^16 long, same for cipher suites, and you can't have
> > > >> data
> > > >> trailing the messages, so the actual size is limited to something
> > > >> closer 2^18, so if the client hello parser is correct, it will
> > > >> be limited by it> > 
> > > > Yeah, but OpenSSL first tries to "get" the handshake body and its
> > > > length before parsing it (this is done by ssl3_get_message()). So
> > > > the "max" argument is intended to be used, I imagine, as a sanity
> > > > check: if the message exceeds that, then it's obviously broken
> > > > and an "illegal parameters" alert is sent. This is done
> > > > regardless of the message type, so the ClientHello parser has to
> > > > do this as well.
> > > > 
> > > > This max length check is not exactly smart (e.g. the max size of
> > > > the SSLv3 ClientHello is very different from that of TLS) and
> > > > could probably be removed completely, but I don't really know
> > > > what the consequences of this would be. So the best next fix
> > > > would simply be to provide an approximation of an absolute
> > > > maximum length for the ClientHello (or just use 0xFF). I
> > > > opened a pull request [0] with just this minimal fix. Anyone is
> > > > very welcome to propose a better fix for this though.
> > > 
> > > 0xff = 16777215 or 16Mb
> > > 
> > > Allowing a ClientHello as big as this could enable a DoS attack.
> > > 
> > > If I did my sums right I make the biggest possible valid ClientHello
> > > to be 131396.
> > 
> > I updated my patch to use this value now.
> 
> embedding magic values is rather bad form. 
> A define with extended comment describing how we arrived at this value 
> would be much better

Fair enough. Updated the patch again to do this.

Cheers


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

[openssl-dev] [openssl.org #4062] [PATCH] Fix build failure

[Alessandro Ghedini via RT]

Hello,

due to commit a93d3e0 the ./config script currently fails with the error:

> Operating system: x86_64-whatever-linux2
> This system (linux-x86_64) is not supported. See file INSTALL for details.

see the following GitHub pull request for a fix:
https://github.com/openssl/openssl/pull/412

Cheers

___
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 #4063] Client Hello longer than 2^14 bytes are rejected

[Alessandro Ghedini via RT]

On Fri, Sep 25, 2015 at 01:20:12pm +, Hubert Kario via RT wrote:
> Current OpenSSL-1.0.1, 1.0.2 as well as state-machine-rewrite branches 
> reject Client Hello messages bigger than 2^14+4 bytes.

IIRC SSLv3 does place the limit at 2^14 or so bytes, so I think the problem is
that OpenSSL only checks for that.

AFAICT both SSLv3 and TLS implementations share the same ssl_accept() method
(that is ssl3_accept()), which calls e.g. ssl3_get_client_key_exchange() which
in turn calls the ssl_get_message() method (implemented by ssl3_get_message())
using SSL3_RT_MAX_PLAIN_LENGTH as maximum size.

I think a proper fix would be to have all the ssl_get_message() calls changed
to use the proper "max" parameter depending on the protocol version.

The above applies to current master, I haven't checked the state machine
rewrite branch yet.

I can look into preparing a patch, if no one beats me to it.

Cheers


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

Re: [openssl-dev] [openssl.org #4048] [PATCH] Fix potential read buffer overflow in PACKET_strndup()

[Alessandro Ghedini via RT]

The GitHub pull request was merged, so this can be closed.

Cheers


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

[openssl-dev] [openssl.org #4052] [PATCH] Print debug info for extended master secret extension

[Alessandro Ghedini via RT]

Hello,

see GitHub pull request at
https://github.com/openssl/openssl/pull/404

This is like RT#4016, but for extended master secret.

Cheers

___
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 #4048] [PATCH] Fix potential read buffer overflow in PACKET_strndup()

[Alessandro Ghedini via RT]

Hello,

see GitHub pull request at
https://github.com/openssl/openssl/pull/399

It provides a short analysis of the problem and a fix.

Cheers

___
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 #3986] [PATCH] Implement HKDF algorithm (RFC 5869)

[Alessandro Ghedini via RT]

Hello,

FYI I rebased the code [0] on master and updated it to use the new test suite
framework. As mentioned in the GitHub PR, I kept the actual implementation and
the tests on two separate commits for easier review, but if you prefer I can
squash them together.

Could someone please review this? Since this is gonna be needed to implement
TLS 1.3 in the future, I don't think there's any objection against merging
this functionality.

Cheers

[0] https://github.com/openssl/openssl/pull/355


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

Re: [openssl-dev] [openssl.org #1542] others quick patches for memory leaks in pk7_smime.c and pk7_mime.c

[Alessandro Ghedini via RT]

The proposed patch is mangled and very hard to read, but I think all proposed
changes have already been committed, or the code has been removed.

So I think this can be closed now.

Cheers


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

Re: [openssl-dev] [openssl.org #1543] memory leak in crypto/asn1/x_x509a.c

[Alessandro Ghedini via RT]

Same as #1542, the patch is mangled but I think everything is already fixed so
this can be closed.

Cheers


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

Re: [openssl-dev] [openssl.org #4030] Re: [openssl-dev #1542] others quick patches for memory leaks in pk7_smime.c and pk7_mime.c

[Alessandro Ghedini via RT]

On Sat, Sep 05, 2015 at 01:49:23pm +, Alessandro Ghedini via RT wrote:
> The proposed patch is mangled and very hard to read, but I think all proposed
> changes have already been committed, or the code has been removed.
> 
> So I think this can be closed now.

Ugh, wrong subject... this was supposed to be a reply to RT#1542 and can be
closed. Sorry for the noise.

Cheers


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

Re: [openssl-dev] [openssl.org #4031] Re: [openssl-dev #1543] memory leak in crypto/asn1/x_x509a.c

[Alessandro Ghedini via RT]

On Sat, Sep 05, 2015 at 01:49:52pm +, Alessandro Ghedini via RT wrote:
> Same as #1542, the patch is mangled but I think everything is already fixed so
> this can be closed.

Same as #4031. It was supposed to be a reply to #1543 and can be closed.

Cheers


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

[openssl-dev] [openssl.org #4031] Re: [openssl-dev #1543] memory leak in crypto/asn1/x_x509a.c

[Alessandro Ghedini via RT]

Same as #1542, the patch is mangled but I think everything is already fixed so
this can be closed.

Cheers

___
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 #4030] Re: [openssl-dev #1542] others quick patches for memory leaks in pk7_smime.c and pk7_mime.c

[Alessandro Ghedini via RT]

The proposed patch is mangled and very hard to read, but I think all proposed
changes have already been committed, or the code has been removed.

So I think this can be closed now.

Cheers

___
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 #3985] [PATCH] Fix potential memory leaks

[Alessandro Ghedini via RT]

The corresponding GitHub pull request was merged, so this can be closed now.

Cheers


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

[openssl-dev] [openssl.org #4016] [PATCH] Print debug info for ALPN extension

[Alessandro Ghedini via RT]

Hello,

see GitHub pull request at
https://github.com/openssl/openssl/pull/371

Which simply adds ALPN to the -tlsextdebug output, so that the extension is
not shown as unknown.

Cheers

___
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 #4017] [PATCH] Implement Camellia GCM suites (RFC 6367)

[Alessandro Ghedini via RT]

Hello,

see GitHub pull request at
https://github.com/openssl/openssl/pull/374

Which adds support for Camellia GCM and adds the correspondent TLS cipher
suites. Most of the code comes from the AES GCM implementation, so maybe
there's an opportunity for some refactoring there.

This fixes issue #320 on GitHub
https://github.com/openssl/openssl/issues/320

Cheers

___
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 #4017] [PATCH] Implement Camellia GCM suites (RFC 6367)

[Alessandro Ghedini via RT]

On Sat, Aug 22, 2015 at 01:17:36PM +, Stephen Henson via RT wrote:
 On Sat Aug 22 10:21:42 2015, alessan...@ghedini.me wrote:
  Hello,
 
  see GitHub pull request at
  https://github.com/openssl/openssl/pull/374
 
  Which adds support for Camellia GCM and adds the correspondent TLS cipher
  suites. Most of the code comes from the AES GCM implementation, so maybe
  there's an opportunity for some refactoring there.
 
 
 Note that the AES-GCM IV generation is purely there to satisfy the FIPS
 requirements. Since Camellia doesn't have such requirements it could instead
 use the sequence number directly and remove the generation, simplifying the
 code in the process. The recently added AES-CCM code does this.

Ok. I removed the IV generation now, and everything seems to work fine (I've
also done some tests with gnutls as well), but more testing may be needed.

Cheers


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

[openssl-dev] [openssl.org #3985] [PATCH] Fix potential memory leaks

[Alessandro Ghedini via RT]

Hello,

see GitHub pull request at
https://github.com/openssl/openssl/pull/354

which fixes memory leaks on error conditions in X509_add1_reject_object()
and PKCS7_verify().

Cheers

___
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 #3986] [PATCH] Implement HKDF algorithm (RFC 5869)

[Alessandro Ghedini via RT]

Hello,

see GitHub pull request at
https://github.com/openssl/openssl/pull/355

which implements the HMAC-based Extract-and-Expand Key Derivation Function
(HKDF) as defined in RFC 5869, and used by QUIC and TLS 1.3.

It comes with tests as defined in the Appendix A of the same RFC.

Cheers

___
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 #3985] [PATCH] Fix potential memory leaks

[Alessandro Ghedini via RT]

On Wed, Aug 05, 2015 at 11:01:13am +, Alessandro Ghedini via RT wrote:
 Hello,
 
 see GitHub pull request at
 https://github.com/openssl/openssl/pull/354
 
 which fixes memory leaks on error conditions in X509_add1_reject_object()
 and PKCS7_verify().

I also added a couple more patches fixing memory leaks in the test suite. Now
it's possible to run make test with GCC's address sanitizer, without any
errors.


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

Re: [openssl-dev] [openssl.org #3668] [PATCH] Don't use the cert list embedded in the OCSP response to build the trust chain

[Alessandro Ghedini via RT]

On Tue, Mar 24, 2015 at 01:19:31PM +0100, Stephen Henson via RT wrote:
 On Fri Mar 20 13:20:07 2015, alessan...@ghedini.me wrote:
 
  Months have passed and I haven't received a reply yet (even worse, the
  recent
  obfuscation of the OCSP structures in 6ef869d7d0a9d made it impossible
  to
  workaround the issue as curl has been doing [0]), so I thought I'd add
  some more
  information to hopefully have this issue resolved.
 
 
 Sorry for the delay in responding. Unfortunately I can't apply your patch
 because it would break any applications which rely on the existing behaviour. 
 I
 have just committed a change which will concatenate the supplied certificates
 with any internal ones. This should address your problem (the test program now
 works) and retain compatibility.

Thanks, your patch works for me too. I guess this bug can be closed now.

Cheers


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

Re: [openssl-dev] [openssl.org #3668] [PATCH] Don't use the cert list embedded in the OCSP response to build the trust chain

[Alessandro Ghedini via RT]

On mar, gen 20, 2015 at 02:31:14 +0100, Alessandro Ghedini wrote:
 Currently the OCSP_basic_verify() function fails with many apparently valid 
 OCSP
 responses (e.g. all those sent by Cloudflare servers). Other libraries 
 (GnuTLS,
 NSS) have no problem with them.
 
 Essentially, in crypto/ocsp/ocsp_vfy.c in the OCSP_basic_verify() function, 
 the
 X509_STORE_CTX_init() function is called like this:
 
   init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);
 
 where ctx is the X509_STORE_CTX to be initialized, st is the trust store 
 passed
 by the user, signer is the signer of the OCSP response (which is what needs to
 be validated), and bs is the decoded OCSP basic response.
 
 The problem is the last argument. OpenSSL uses the cert list embedded in the
 OCSP response to build the trust chain, but it seems that in some cases this
 list is somewhat broken. Other libraries (e.g. GnuTLS), do the verification
 differently, without including those bs-certs that OpenSSL uses.
 
 I attached the patch and a simple test case. You can compile it with:
 
   $ cc ocsp_test.c -lcrypto -lssl
 
 To test the problem run:
 
   $ ./a.out digitalocean.com 443
   OCSP response verification failed
 
 after the patch:
 
   $ ./a.out digitalocean.com 443
   OK

Months have passed and I haven't received a reply yet (even worse, the recent
obfuscation of the OCSP structures in 6ef869d7d0a9d made it impossible to
workaround the issue as curl has been doing [0]), so I thought I'd add some more
information to hopefully have this issue resolved.

The issue affects servers that have an intermediate certificate (so the peer
chain is A - B - C, with C being the peer cert, B the intermediate cert issuer
of C, and A the CA cert issuer of B) when the OCSP response is signed by an
additional certificate issued by B (lets call this X), provided by the peer in
the certs field of the OCSP basic reponse (bs-certs in the OpenSSL code).

To put this in drawing:

peer
chain bs-certs
  (certs)

  A
  ↓
  B--┐
  ↓  ↓
  C  X

In this case OpenSSL tries to verify that X is trusted, by using the
user-provided trust store st (OK), and the bs-certs stack (which only
includes X itself) *without* including B (that the user is supposed to provide
with the certs parameter of OCSP_basic_verify()) which actually issued X, thus
failing miserably.

This is the case for e.g. all CloudFlare sites AFAICT, and possibly many other
certificate authorities.

Cheers

[0] https://github.com/bagder/curl/blob/2b7ac4e/lib/vtls/openssl.c#L1363-L1385


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

Re: [openssl-dev] [openssl.org #3668] [PATCH] Don't use the cert list embedded in the OCSP response to build the trust chain

[Alessandro Ghedini via RT]

On mar, gen 20, 2015 at 02:31:14 +0100, Alessandro Ghedini wrote:
 Currently the OCSP_basic_verify() function fails with many apparently valid 
 OCSP
 responses (e.g. all those sent by Cloudflare servers). Other libraries 
 (GnuTLS,
 NSS) have no problem with them.
 
 Essentially, in crypto/ocsp/ocsp_vfy.c in the OCSP_basic_verify() function, 
 the
 X509_STORE_CTX_init() function is called like this:
 
   init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);
 
 where ctx is the X509_STORE_CTX to be initialized, st is the trust store 
 passed
 by the user, signer is the signer of the OCSP response (which is what needs to
 be validated), and bs is the decoded OCSP basic response.
 
 The problem is the last argument. OpenSSL uses the cert list embedded in the
 OCSP response to build the trust chain, but it seems that in some cases this
 list is somewhat broken. Other libraries (e.g. GnuTLS), do the verification
 differently, without including those bs-certs that OpenSSL uses.
 
 I attached the patch and a simple test case. You can compile it with:
 
   $ cc ocsp_test.c -lcrypto -lssl
 
 To test the problem run:
 
   $ ./a.out digitalocean.com 443
   OCSP response verification failed
 
 after the patch:
 
   $ ./a.out digitalocean.com 443
   OK

I updated the patch so that it applies cleanly after the reformatting of
ocsp_vfy.c in commit 0f113f3.

Cheers

From 7d18036c5469305719993c0080be71dfb5c50966 Mon Sep 17 00:00:00 2001
From: Alessandro Ghedini alessan...@ghedini.me
Date: Sat, 31 Jan 2015 17:01:54 +0100
Subject: [PATCH] Don't use the cert list embedded in the OCSP response to
 build the trust chain

Instead use the certificate stack passed by the user.

This is required in some cases where the embedded chain is incomplete and causes
the OCSP verification to fail, e.g. all DigiCert/Cloudflare sites.
---
 crypto/ocsp/ocsp_vfy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/ocsp/ocsp_vfy.c b/crypto/ocsp/ocsp_vfy.c
index 6c0ccb5..ef1d253 100644
--- a/crypto/ocsp/ocsp_vfy.c
+++ b/crypto/ocsp/ocsp_vfy.c
@@ -110,7 +110,7 @@ int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs,
 if (flags  OCSP_NOCHAIN)
 init_res = X509_STORE_CTX_init(ctx, st, signer, NULL);
 else
-init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);
+init_res = X509_STORE_CTX_init(ctx, st, signer, certs);
 if (!init_res) {
 ret = -1;
 OCSPerr(OCSP_F_OCSP_BASIC_VERIFY, ERR_R_X509_LIB);
-- 
2.1.4

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

Re: [openssl-dev] [openssl.org #3668] [PATCH] Don't use the cert list embedded in the OCSP response to build the trust chain

[Alessandro Ghedini via RT]

On mar, gen 20, 2015 at 02:31:14 +0100, Alessandro Ghedini wrote:
 Currently the OCSP_basic_verify() function fails with many apparently valid 
 OCSP
 responses (e.g. all those sent by Cloudflare servers). Other libraries 
 (GnuTLS,
 NSS) have no problem with them.
 
 Essentially, in crypto/ocsp/ocsp_vfy.c in the OCSP_basic_verify() function, 
 the
 X509_STORE_CTX_init() function is called like this:
 
   init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);
 
 where ctx is the X509_STORE_CTX to be initialized, st is the trust store 
 passed
 by the user, signer is the signer of the OCSP response (which is what needs to
 be validated), and bs is the decoded OCSP basic response.
 
 The problem is the last argument. OpenSSL uses the cert list embedded in the
 OCSP response to build the trust chain, but it seems that in some cases this
 list is somewhat broken. Other libraries (e.g. GnuTLS), do the verification
 differently, without including those bs-certs that OpenSSL uses.
 
 I attached the patch and a simple test case. You can compile it with:
 
   $ cc ocsp_test.c -lcrypto -lssl
 
 To test the problem run:
 
   $ ./a.out digitalocean.com 443
   OCSP response verification failed
 
 after the patch:
 
   $ ./a.out digitalocean.com 443
   OK

Ping? This is actually pretty important since OpenSSL can't verify OCSP
responses from a whole bunch of servers.

Cheers


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

Re: [openssl-dev] [openssl.org #3668] [PATCH] Don't use the cert list embedded in the OCSP response to build the trust chain

[Alessandro Ghedini via RT]

On mar, gen 20, 2015 at 02:31:14 +0100, Alessandro Ghedini wrote:
 Currently the OCSP_basic_verify() function fails with many apparently valid 
 OCSP
 responses (e.g. all those sent by Cloudflare servers). Other libraries 
 (GnuTLS,
 NSS) have no problem with them.
 
 Essentially, in crypto/ocsp/ocsp_vfy.c in the OCSP_basic_verify() function, 
 the
 X509_STORE_CTX_init() function is called like this:
 
   init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);
 
 where ctx is the X509_STORE_CTX to be initialized, st is the trust store 
 passed
 by the user, signer is the signer of the OCSP response (which is what needs to
 be validated), and bs is the decoded OCSP basic response.
 
 The problem is the last argument. OpenSSL uses the cert list embedded in the
 OCSP response to build the trust chain, but it seems that in some cases this
 list is somewhat broken. Other libraries (e.g. GnuTLS), do the verification
 differently, without including those bs-certs that OpenSSL uses.
 
 I attached the patch and a simple test case. You can compile it with:
 
   $ cc ocsp_test.c -lcrypto -lssl
 
 To test the problem run:
 
   $ ./a.out digitalocean.com 443
   OCSP response verification failed
 
 after the patch:
 
   $ ./a.out digitalocean.com 443
   OK

Ping? This is actually pretty important since OpenSSL can't verify OCSP
responses from a whole bunch of servers.

Cheers


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

[openssl-dev] [openssl.org #3668] [PATCH] Don't use the cert list embedded in the OCSP response to build the trust chain

[Alessandro Ghedini via RT]

Currently the OCSP_basic_verify() function fails with many apparently valid OCSP
responses (e.g. all those sent by Cloudflare servers). Other libraries (GnuTLS,
NSS) have no problem with them.

Essentially, in crypto/ocsp/ocsp_vfy.c in the OCSP_basic_verify() function, the
X509_STORE_CTX_init() function is called like this:

  init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);

where ctx is the X509_STORE_CTX to be initialized, st is the trust store passed
by the user, signer is the signer of the OCSP response (which is what needs to
be validated), and bs is the decoded OCSP basic response.

The problem is the last argument. OpenSSL uses the cert list embedded in the
OCSP response to build the trust chain, but it seems that in some cases this
list is somewhat broken. Other libraries (e.g. GnuTLS), do the verification
differently, without including those bs-certs that OpenSSL uses.

I attached the patch and a simple test case. You can compile it with:

  $ cc ocsp_test.c -lcrypto -lssl

To test the problem run:

  $ ./a.out digitalocean.com 443
  OCSP response verification failed

after the patch:

  $ ./a.out digitalocean.com 443
  OK

Cheers

From 38ce9b993e9ffc76416ccdc26dee53b24b2cd33c Mon Sep 17 00:00:00 2001
From: Alessandro Ghedini alessan...@ghedini.me
Date: Tue, 20 Jan 2015 12:27:00 +0100
Subject: [PATCH] Don't use the cert list embedded in the OCSP response to
 build the trust chain

Instead use the certificate stack passed by the user.

This is required in some cases where the embedded chain is somewhat broken and
causes the OCSP verification to fail, e.g. all DigiCert/Cloudflare sites.
---
 crypto/ocsp/ocsp_vfy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/ocsp/ocsp_vfy.c b/crypto/ocsp/ocsp_vfy.c
index fc0d4cc..846971e 100644
--- a/crypto/ocsp/ocsp_vfy.c
+++ b/crypto/ocsp/ocsp_vfy.c
@@ -108,7 +108,7 @@ int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs,
 		if(flags  OCSP_NOCHAIN)
 			init_res = X509_STORE_CTX_init(ctx, st, signer, NULL);
 		else
-			init_res = X509_STORE_CTX_init(ctx, st, signer, bs-certs);
+			init_res = X509_STORE_CTX_init(ctx, st, signer, certs);
 		if(!init_res)
 			{
 			ret = -1;
-- 
2.1.4

#include stdio.h
#include netdb.h

#include openssl/ssl.h
#include openssl/ocsp.h

int main(int argc, char *argv[]) {
	int sd;

	SSL *ssl;
	BIO *sbio;
	SSL_CTX *ctx;

	SSL_library_init();
	SSL_load_error_strings();

	ctx = SSL_CTX_new(SSLv23_client_method());

	SSL_CTX_load_verify_locations(ctx, NULL, /etc/ssl/certs);

	sd = tcp_connect(argv[1], argv[2]);

	ssl = SSL_new(ctx);

	SSL_set_fd(ssl, (int) sd);
	SSL_set_tlsext_status_type(ssl, TLSEXT_STATUSTYPE_ocsp);

	if (SSL_connect(ssl) = 0) {
		puts(SSL connect error);
		exit(-1);
	}

	if (SSL_get_verify_result(ssl) != X509_V_OK) {
		puts(Certificate doesn't verify);
		exit(-1);
	}

	/*  VERIFY OCSP RESPONSE  */

	int i, ocsp_status;
	const unsigned char *p;

	OCSP_RESPONSE *rsp = NULL;
	OCSP_BASICRESP *br = NULL;
	X509_STORE *st = NULL;
	STACK_OF(X509) *ch = NULL;

	long len = SSL_get_tlsext_status_ocsp_resp(ssl, p);

	if (!p) {
		puts(No OCSP response received);
		exit(-1);
	}

	rsp = d2i_OCSP_RESPONSE(NULL, p, len);
	if (!rsp) {
		puts(Invalid OCSP response);
		exit(-1);
	}

	ocsp_status = OCSP_response_status(rsp);
	if (ocsp_status != OCSP_RESPONSE_STATUS_SUCCESSFUL) {
		printf(Invalid OCSP response status: %s (%d),
		   OCSP_response_status_str(ocsp_status), ocsp_status);
		exit(-1);
	}

	br = OCSP_response_get1_basic(rsp);
	if (!br) {
		puts(Invalid OCSP response);
		exit(-1);
	}

	ch = SSL_get_peer_cert_chain(ssl);
	st = SSL_CTX_get_cert_store(ctx);

	if (OCSP_basic_verify(br, ch, st, 0) = 0) {
		puts(OCSP response verification failed);
		exit(-1);
	}

	puts(OK);

	return 0;
}

int tcp_connect(char *host, char *port) {
	int err, sd;
	struct addrinfo hints, *res, *r;

	memset(hints, 0, sizeof(struct addrinfo));
	hints.ai_family = AF_INET;
	hints.ai_socktype = SOCK_STREAM;

	err = getaddrinfo(host, port, hints, res);
	if (err != 0) {
		perror(getaddrinfo());
		exit(-1);
	}

	for (r = res; r != NULL; r = r-ai_next) {
		sd = socket(r-ai_family, r-ai_socktype, r-ai_protocol);
		if (sd == -1)
			continue;

		if (connect(sd, r-ai_addr, r-ai_addrlen) == 0)
			break;

		close(sd);
	}

	freeaddrinfo(res);

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