Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[David Benjamin via RT]

I may not have time to fully digest the change before the release date, but
I'm not sure this snippet quite works:

   if (ctx->read_start == ctx->read_end) { /* time to read more data */
   ctx->read_end = ctx->read_start = &(ctx->buf[BUF_OFFSET]);
   ctx->read_end += BIO_read(next, ctx->read_start, ENC_BLOCK_SIZE);
   }
   i = ctx->read_end - ctx->read_start;

   if (i <= 0) {

It's kind of an odd error-checking pattern and is only saved from undefined
behavior by BUF_OFFSET. (Is a custom BIO allowed to return -1,000,000 on
error or must it be -1? There are definitely some OpenSSL APIs which return
-2 expecting that the usual error-check patterns don't care.) Anyway, I
believe it gets stuck if non-blocking BIO causes BIO_read to fail on a
retryable error like EWOULDBLOCK and we try again. I see calls to
BIO_should_retry, so I gather this BIO is intended to work in front of a
non-blocking BIO.

Since the error path should only be reachable when BIO_read fails, maybe
move that inside the "read more data" codepath? Then you don't need pointer
tricks to avoid duplicating the code.

David

On Sun, Aug 21, 2016 at 5:57 PM Andy Polyakov via RT  wrote:

> There are two commits, one that addresses bio_enc problems and one
> adding test. Please double-check.
>
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
> Please log in as guest with password guest if prompted
>
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
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 #4628] EVP_f_cipher regression due to overlapping regions check

[David Benjamin via RT]

On Sun, Jul 31, 2016 at 6:18 PM Michel via RT  wrote:

> > I was able to trigger a crash simply by chaining an encrypt BIO with a
> memory BIO containing a large plaintext and then stream 100 bytes out of it
> at a time. BIO_read would consistently return 128 and, by the time the
> function returned, the stack was thoroughly clobbered.
>
> I am surprised. I should have been [un-?]lucky for once.
> FWIW, here is what I did :
> I have some files containing about 1 of variable length lines (range is
> from about 60 to 260 bytes).
> File size is about 900 Kb to 1.5 Mb.
> Files can be cleartext or encrypted (in this case they can be optionaly
> base64 encoded).
> So I have a software that chain as follow :
> File bio ->
> Base64 bio (opt) ->
> Cipher bio (opt) ->
> Memory bio.
>
> For my test I read and wrote each case using 2 different ciphers : aes-128
> and camelia-192.
> Everything looks good : no crash, no data lost or damaged, no memory leak
> and no stack overwritten.
>
> I certainly misunderstand something, but I will be happy to test again my
> use case if it can be of any help.
>

(I'm not entirely sure which direction the arrows are meant to be going.)

You need the read to not be a multiple of 16 to trigger the issue. (Well,
that's the simplest trigger. I also got decrypt BIOs to trigger with
outl=128 because EVP_DecryptUpdate always holds back the final block.) Also
if your code will secretly tolerate the cipher BIO returning more data than
outl, you won't notice. But the API contract of BIO_read is that it will
not return more than outl bytes of data.

Reads of size 100 into a buffer of size 100 will do the trick:

outl = 100 =>
buf_len = 128 =>
i = 128 (assuming we got a full read) =>
we ask EVP_CipherUpdate to decrypt 128 bytes =>
128 bytes of output into out =>
buffer overflow

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
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 #4628] EVP_f_cipher regression due to overlapping regions check

[David Benjamin via RT]

Hey folks,

I do not believe this fix works and introduces buffer overflows.

Looking at this change:
https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=abdb460d8abe68fedcf00b52d2ba4bf4b7c1725c

It makes EVP_CipherUpdate write to out directly. While not unreasonable
(this BIO probably should never buffer more than a block and otherwise use
the buffer passed in), one must take care to never output more than outl
bytes. There are a number of problems here:

1. The logic to compute buf_len above rounds outl *up* to a multiple of
EVP_MAX_BLOCK_LENGTH. Thus, if outl > buf_len and BIO_read returns the full
buf_len bytes, EVP_CipherUpdate, may write up to buf_len bytes and exceed
outl.

2. If the EVP_CIPHER_CTX contains a partial block buffered, even if outl ==
buf_len, we may *still* output more than outl bytes. For instance, the BIO
may be connected to a pipe and BIO_read return less than buf_len. That will
feed a partial block into the EVP_CIPHER_CTX and, the next time around, we
output more data than expected.

3. Actually, #2 even means the EVP_CIPHER overlapping buffers check is
wrong. The true requirement is not "if the buffers alias, then in == out",
but "if the buffers alias, then out + ctx->num == in". EVP_CIPHER's block
cipher handling is a very leaky abstraction.

I was able to trigger a crash simply by chaining an encrypt BIO with a
memory BIO containing a large plaintext and then stream 100 bytes out of it
at a time. BIO_read would consistently return 128 and, by the time the
function returned, the stack was thoroughly clobbered.

#3 suggests a very minimal fix. Revert the direct output codepath (not
wrong, but I think the BIO needs a complete rewrite with a block-size
buffer for that sort of thing anyway). Then, instead of reading BUF_OFFSET
bytes in at the BIO_read call, read ctx->num bytes in. Then go and fix the
EVP_CIPHER overlap check so it handles the ctx->num != 0 case correctly.

NB: I'm unclear on what the BUF_OFFSET offset was originally for. The
comment just says to read the EVP_Cipher documentation, but there is no pod
file for EVP_Cipher. I am assuming it was to get around this partial block
problem, but perhaps I'm missing something and my suggestion is also wrong.

Hope this helps,

David

PS: Should the new codepath have been setting ctx->cont? The others do.
Though it looks like it might be a no-op when set to 1? I'm not sure.

PPS: This sort of streaming stuff is quite hairy. Like the poly1305
streaming bits, it would make sense to write some tests here. Get a long
plaintext/ciphertext vector or two (I'd test both a block cipher and a
stream cipher) and make sure everything behaves correctly against various
interesting read patterns. Both when the outl pattern varies and when the
inner BIO_read return pattern varies.

On Sun, Jul 31, 2016 at 1:53 PM Rich Salz via RT  wrote:

> Resolved by Andy's fix. Closing.
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
> Please log in as guest with password guest if prompted
>
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
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 #4572] SSL_set_bio and friends

[David Benjamin via RT]

On Fri, Jul 29, 2016 at 9:21 AM Matt Caswell via RT  wrote:

> On Tue Jun 14 20:30:09 2016, david...@google.com wrote:
> > I recently made some changes around BoringSSL's SSL_set_bio, etc.
> > which you
> > all might be interested in. The BIO management has two weird behaviors
> > right now:
> >
> > 1. The existence of bbio is leaked in the public API when it should be
> > an
> > implementation detail. (Otherwise you're stuck with it for DTLS where
> > it's
> > really messy.) SSL_get_wbio will return it, and SSL_set_bio messes up
> > when
> > the bbio is active.
>
> Fixed by 2e7dc7cd688.
>
> > 2. SSL_set_bio's object ownership story is a mess. It also doesn't
> > quite
> > work. This crashes:
> > SSL_set_fd(ssl, 1);
> > SSL_set_rfd(ssl, 2);
> > But this does not:
> > SSL_set_fd(ssl, 1);
> > SSL_set_wfd(ssl, 2);
> > Not that anyone would do such a thing, but the asymmetry is off.
>
> Fixed by 2e7dc7cd688 and in the docs by e040a42e44.
>
> I also added a test, which I verified against the original 1.0.2
> implementation
> of SSL_set_bio(), in 7fb4c82035.
>
> I found I needed to make some tweaks to the implementation of SSL_set_bio()
> from your version in order to preserve the behaviour between 1.0.2 and
> master.
> Possibly your version was a deliberate simplification.


Hrm. It's been a while, but I believe it was a deliberate simplification to
fix the SSL_set_rfd crash above.

My interpretation was that SSL_set_rfd and SSL_set_wfd's calling
pattern, insane
as it is, is definitively correct. Thus, the asymmetry in SSL_set_bio is a
bug and was a deliberate change on my part.

It seems your interpretation was that SSL_set_bio's behavior, insane as it
is, is definitively correct. Thus the bug lies in SSL_set_[rw]fd using
SSL_set_bio wrong, so you changed those functions and kept SSL_set_bio's
behavior as-is. (I'm not sure SSL_set_bio actually lets you implement
SSL_set_rfd, but you have the new side-specific setters and just used that.)

I like my interpretation slightly better if only because it makes
SSL_set_bio's behavior a little more sane. Judging by SSL_set_[rw]fd, it
also seems to have been the original intent. It is a behavior change, but
one I'm sure will break no one.

(If you still prefer yours, I will make BoringSSL match since I see no
reason for us to diverge here.)

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4572
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 #4623] OpenSSL master regression in handling malformed Client Key Exchange messages in RSA key exchange

[David Benjamin via RT]

On Fri, Jul 22, 2016 at 7:30 PM Hubert Kario via RT  wrote:

> On Friday, 22 July 2016 17:14:43 CEST Stephen Henson via RT wrote:
> > On Fri Jul 22 14:56:11 2016, hka...@redhat.com wrote:
> > > the issue is present in master 0ed26acce328ec16a3aa and looks to have
> > > been
> >
> > > introduced in commit:
> > I tried what I thought was a fix for this which is to simply delete the
> > lines:
> >
> > if (decrypt_len < 0)
> > goto err;
> >
> > from ssl/statem/statem_srvr.c
> >
> > However your reproducer still indicates errors. I checked the message
> logs
> > and it should be now sending as many alerts as the original. The
> difference
> > however is that some of them will be sent immediately whereas originally
> > they would be at the end of the handshake.
> >
> > Could your reproducer possibly not be expecting this?
>
>
> sorry, I copied this part:
>
> > when the OpenSSL receives a Client Key Exchange message that has the
> > encrypted
> > premaster secret comprised only of zero bytes, or equal to server's
> modulus,
> > the server just aborts the connection without sending an Alert message
>
> from the DHE/ECDHE bug reports
>
> the expected behaviour is to continue the connection, but the server should
> select a premaster secret that was not provided by the client, instead
> OpenSSL
> just closes the connection
>

I don't believe this test is correct if the encrypted premaster is equal to
the server's modulus. Such an encrypted premaster is a publicly invalid RSA
ciphertext. It is perfectly reasonable to reject it early, should unpadded
RSA_decrypt fail. The timing sensitive portion is the padding check itself,
which this code should correctly defer failure of, to avoid the padding
oracle.

The reason public decrypt failures did not trigger early alerts before was
because the old code was a little sloppy about error-handling. In addition
to not being constant time, it squashed together codepaths for publicly
invalid keys and the timing sensitive check.

It is true that it no longer sends an alert on public failures. Probably
worth fixing. Meh.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4623
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 #4572] SSL_set_bio and friends

[David Benjamin via RT]

On Fri, Jun 17, 2016 at 8:48 AM Matt Caswell via RT <r...@openssl.org> wrote:

>
>
> On 14/06/16 21:30, David Benjamin via RT wrote:
> > For OpenSSL master, I believe it'd also work to add an s->rbio != s->wbio
> > check to SSL_set_rbio, but I think those are worse semantics for
> > SSL_set_{rbio,wbio}. They are new APIs, so, before it's too late, give
> them
> > clear semantics like "SSL_set_rbio takes ownership of its argument",
> > consistent with "set0" functions, rather than a mix of "set0" and "set1".
>
> These look like good changes. I'm wondering whether we should actually
> rename SSL_set_rbio() and SSL_set_wbio() to SSL_set0_rbio() and
> SSL_set0_wbio() - especially since they are new to 1.1.0 so not released
> yet.
>

Sounds good to me.


> *Possibly* we could also rename SSL_set_bio() to SSL_set0_bio() with a
> deprecated compatibility macro.
>

I dunno, SSL_set_bio is kind of weird all around. :-) I suppose it is
closer to a set0 than a set1, but set0 doesn't typically have all these
no-op cases around taking ownership and such.


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

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4572
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 #4577] X509_LU_RETRY and X509_LU_FAIL are slightly confused

[David Benjamin via RT]

Hey folks,

It seems the X509_LU_* values (now an X509_LOOKUP_TYPE enum in master) are
slightly confused. See this commit message and diff for details:
https://boringssl.googlesource.com/boringssl/+/da7f0c65efb72556f8fc92e460e6c90cd1b1add7%5E%21/

The relevant point is that X509_LU_RETRY doesn't work and has never worked
since SSLeay. The free of an uninitialized pointer seems to have gotten
fixed as a consequence of how X509_OBJECT was opaquified, but the
current_method = j bug is still there. It's possible that's all that's
needed to fix it, but I doubt it. (Come to think of it, I bet
aae41f8c54257d9fa6904d3a9aa09c5db6cefd0d
destroyed any hope of X509_LU_RETRY ever working again...)

I would propose that you all do something similar to the BoringSSL change
above, especially since 1.1.0 is allowed to break API. Remove X509_LU_RETRY
support and both error enums completely. X509_LOOKUP_TYPE is now just a
type enum (then you can remove the default cases added in
bca3f06b84de3c0b428724ac535995064c54aee3). The functions in question just
return 0/1 (or -1/0/1, but I think the less error-prone 0/1 is generally
better).

David

PS: It appears bca3f06b84de3c0b428724ac535995064c54aee3
removed X509_LU_PKEY without removing X509_OBJECT.data.pkey. That too can
probably go.

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4577
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 #4362] chacha-x86.pl has stricter aliasing requirements than other files

[David Benjamin via RT]

I don't think that will work. The SSL code uses in-place buffers
extensively, so in == out definitely needs to be defined. The question is
only whether out < in is also acceptable.

Either way, for BoringSSL, I've gone ahead and tightened our aliasing
constraints to forbid out < in and require equality, so that we don't have
to keep chasing down discrepancies in the assembly code in advance of a
decision being made here.

(I think there is something to be said for being able to in-place-ish
decrypt a structure with a record header and write the output without the
header, but perhaps this use case is not worth the cost---I see the numbers
went down slightly for chacha-x86.pl. Then again, most other files manage
it naturally. It's a decision you all will need to make.)

David


On Wed, Jun 15, 2016 at 11:01 AM Rich Salz via RT  wrote:

> I think for now, we just note this in the documentation: behavior for
> overlapping buffers, and even in-place buffers, is not defined.
>
> It's like memcpy() vs memmove().
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4362
> Please log in as guest with password guest if prompted
>
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4362
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 #4572] SSL_set_bio and friends

[David Benjamin via RT]

I recently made some changes around BoringSSL's SSL_set_bio, etc. which you
all might be interested in. The BIO management has two weird behaviors
right now:

1. The existence of bbio is leaked in the public API when it should be an
implementation detail. (Otherwise you're stuck with it for DTLS where it's
really messy.)  SSL_get_wbio will return it, and SSL_set_bio messes up when
the bbio is active.

2. SSL_set_bio's object ownership story is a mess. It also doesn't quite
work. This crashes:
   SSL_set_fd(ssl, 1);
   SSL_set_rfd(ssl, 2);
But this does not:
   SSL_set_fd(ssl, 1);
   SSL_set_wfd(ssl, 2);
Not that anyone would do such a thing, but the asymmetry is off.

For 1, I made this change:
https://boringssl.googlesource.com/boringssl/+/2f87112b963fe9dee6a75b23a8dae4501063%5E%21/
SSL_get_wbio now always returns the true wbio. BIO_set_bio is aware of bbio
and behaves accordingly.

Then for 2, I wrote this test:
https://boringssl.googlesource.com/boringssl/+/5c0fb889a1348ecaa5691f6139f9d60a610f2129%5E%21/
And then made this change:
https://boringssl.googlesource.com/boringssl/+/f715c423224a292d79ba0e3df373c828fbae29f7%5E%21/
[Plus
comment typo fix]
Releasing ssl->{rbio,wbio} is now much more straight-forward. All the
ownership quirks are left in SSL_set_bio. It's messy, but it's the best
option I found which preserves the existing calling patterns. The different
cases reflect the desired behavior inherently having a lot of cases.

For OpenSSL master, I believe it'd also work to add an s->rbio != s->wbio
check to SSL_set_rbio, but I think those are worse semantics for
SSL_set_{rbio,wbio}. They are new APIs, so, before it's too late, give them
clear semantics like "SSL_set_rbio takes ownership of its argument",
consistent with "set0" functions, rather than a mix of "set0" and "set1".

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4572
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 #4558] Performance issue with DTLS packet reassembly

[David Benjamin via RT]

On Mon, Jun 13, 2016 at 4:04 AM Matt Caswell via RT  wrote:

> On Thu Jun 02 23:24:44 2016, paul.d...@oracle.com wrote:
> > The DTLS packet reassembly code has a performance problem that could
> > result in a DoS attack being possible.
> >
> >
> >
> > The DTLS packet reassembly uses the data structure defined in
> > ssl/pqueue.c for the purpose (it is the only user of this data
> > structure that I can find). This source file implements a priority
> > queue using a singly linked list. This means O(n^2) worst case
> > complexity, where n is the number of fragments. A better, and in fact
> > optimal, solution would be to use a heap for the purpose giving O(n
> > log n) worst case complexity. Doing this would prevent a potential
> > DoS attack.
> >
> >
> >
> > The attack would consist of fragmenting the DTLS stream into as many
> > small packets as possible and sending them in sequential order. Each
> > fragment will require a complete traversal of the list to be added.
> > Continue sending these as long as the DoS is wanted. For reference,
> > changing the list search method or ordering won't prevent such an
> > attack, it just means a different packet ordering is required.
> >
> >
> >
> > Tim Hudson suggested I submit this even though I haven't been able to
> > find time to craft a patch.
>

Were you able to reproduce this performance problem? Note that N is at most
10 here. Assuming the DTLS packet reassembly code manages its queue
correctly (It's rather buggy, but I forget if this was one of its problems.
I eventually gave up trying to digest it and rewrote it from scratch on our
end.), this check will ensure the queue size is tightly bounded:

https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=ssl/statem/statem_dtls.c;h=d75483af6d40ad4c6ed9137eba8a7382a3b0ef0a;hb=HEAD#l634

It could probably be brought down a hair further too. There's no need to
buffer more than the maximum number of messages in a supported handshake
flight.

(pqueue is still a silly data structure to be using here. A fixed-size ring
buffer would be better. Or just a boring array since memmove on 10 pointers
is cheap. But it's not hugely important.)

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4558
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 #4393] [PATCH] Call EC_GROUP_order_bits in priv2opt.

[David Benjamin via RT]

On Tue, Mar 29, 2016 at 12:17 PM Emilia Käsper  wrote:

> While we're at this, shouldn't we then also check the length in oct2priv?
> (And
> either reject or reduce mod n.) Afaics it accepts arbitrary BNs currently,
> which means some keys can be parsed but cannot be re-encoded?
>

Probably. BoringSSL rejects keys that are too large.

One compatibility note though: although RFC 5915 and SEC 1 (not sure about
X9.62) requires that the private key in an ECPrivateKey structure be
exactly the byte length of the order, OpenSSL prior
to 30cd4ff294252c4b6a4b69cbef6a5b4117705d22 removed leading zeros, so
ECPrivateKey parsers need to allow for short inputs.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4393
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 #4483] Wrong results with Poly1305 functions

[David Benjamin via RT]

On Tue, Mar 29, 2016 at 9:47 AM Andy Polyakov via RT  wrote:

> > In the non-SIMD paths, I believe this is fine because $r0's and $r1's
> > cleared high bits mean we should have plenty of slack to leave that
> > unreduced. (And indeed its normally not reduced on input from the
> > addition.) Then poly1305_emit's reduction after adding s will resolve
> > things before output. But, in the SIMD paths, __poly1305_blocks is called
> > and then bits are shifted without any reduction.
>
> What do you mean shifted without any reduction? There is reduction step
> after base 2^26 -> 2^64 conversion (which also needs additional adc, but
> there *is* reduction step) *prior* call to __poly1305_block. And there
> naturally is reduction step at the end of __poly1305_block, so that base
> 2^64 -> 2^26 conversion *after* __poly1305_block is performed at reduced
> value.
>

I mean that here:
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/poly1305/asm/poly1305-x86_64.pl;h=8977d563a25166b5c3bfac9bb952703c40962cfd;hb=HEAD#l535

We call __poly1305_block, which is just poly1305_iteration. If we add the
missing adc, $h2 may exceed two bits, right, so it's not completely
reduced. And the code after the __poly1305_block call above doesn't do an
extra reduction and only shifts bits to convert from 2^64 to 2^26.

I later realized there's plenty of room to spare in the 2^26 representation
even when you put everything in 32-bit values, so we won't lose the extra
bit. I imagine the SIMD logic can tolerate this slightly-unreduced value
just fine, but that was my question.

David


> > Wouldn't that cause a
> > problem? Or is this situation impossible?
>
> If neither of above answers questions, then please elaborate.
>
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4483
> Please log in as guest with password guest if prompted
>
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4483
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 #4483] Wrong results with Poly1305 functions

[David Benjamin via RT]

On Fri, Mar 25, 2016 at 3:26 PM David Benjamin  wrote:

> Right. What I meant is that a fully reduced h has $h2 < 4. Is it possible
> that $h2, after that adc, ends up at 4, exceeding that bound? If it were,
> that would require one more reduction.
>
> In the non-SIMD paths, I believe this is fine because $r0's and $r1's
> cleared high bits mean we should have plenty of slack to leave that
> unreduced. (And indeed its normally not reduced on input from the
> addition.) Then poly1305_emit's reduction after adding s will resolve
> things before output. But, in the SIMD paths, __poly1305_blocks is called
> and then bits are shifted without any reduction. Wouldn't that cause a
> problem? Or is this situation impossible?
>

Pondering this some more, I missed that the base 2^26 representation still
has six bits extra, so we shouldn't immediately lose that bit. How tolerant
is the SIMD code to a partially-reduced h? (I haven't puzzled out how it
works yet.) Is this within its bounds?

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4483
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 #4483] Wrong results with Poly1305 functions

[David Benjamin via RT]

On Fri, Mar 25, 2016 at 3:07 PM Andy Polyakov via RT  wrote:

> > For x86-64, this seems to be the bug:
> >
> > $ git diff
> > diff --git a/crypto/poly1305/asm/poly1305-x86_64.pl
> b/crypto/poly1305/asm/
> > poly1305-x86_64.pl
> > index 3c810c5..bc14ed1 100755
> > --- a/crypto/poly1305/asm/poly1305-x86_64.pl
> > +++ b/crypto/poly1305/asm/poly1305-x86_64.pl
> > @@ -97,6 +97,7 @@ $code.=<<___;
> > add $d3,%rax
> > add %rax,$h0
> > adc \$0,$h1
> > +   adc \$0,$h2
> >  ___
> >  }
>
> Correct. Testing is done on all platforms.
>
> > In the final reduction, $h1 is all ones, so there is one more carry to
> > propagate. Though $h2 can then overflow its two bits, I think? I expect
> > that and the cleared bits of r mean the imulqs in poly1305_iteration are
> > still safe, so we can pick up that slack in poly1305_emit, but I'm not
> sure
> > about all the complex switching back and forth in the SIMD codepaths.
> Does
> > __poly1305_block need to follow up with one more reduction?
>
> That additional adc goes to a perl subroutine that is used in both
> poly1305_blocks and __poly1305_blocks, so modification covers both. Pure
> SIMD paths (or FP) are not affected...
>

Right. What I meant is that a fully reduced h has $h2 < 4. Is it possible
that $h2, after that adc, ends up at 4, exceeding that bound? If it were,
that would require one more reduction.

In the non-SIMD paths, I believe this is fine because $r0's and $r1's
cleared high bits mean we should have plenty of slack to leave that
unreduced. (And indeed its normally not reduced on input from the
addition.) Then poly1305_emit's reduction after adding s will resolve
things before output. But, in the SIMD paths, __poly1305_blocks is called
and then bits are shifted without any reduction. Wouldn't that cause a
problem? Or is this situation impossible?

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4483
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 #4483] Wrong results with Poly1305 functions

[David Benjamin via RT]

For x86-64, this seems to be the bug:

$ git diff
diff --git a/crypto/poly1305/asm/poly1305-x86_64.pl b/crypto/poly1305/asm/
poly1305-x86_64.pl
index 3c810c5..bc14ed1 100755
--- a/crypto/poly1305/asm/poly1305-x86_64.pl
+++ b/crypto/poly1305/asm/poly1305-x86_64.pl
@@ -97,6 +97,7 @@ $code.=<<___;
add $d3,%rax
add %rax,$h0
adc \$0,$h1
+   adc \$0,$h2
 ___
 }

In the final reduction, $h1 is all ones, so there is one more carry to
propagate. Though $h2 can then overflow its two bits, I think? I expect
that and the cleared bits of r mean the imulqs in poly1305_iteration are
still safe, so we can pick up that slack in poly1305_emit, but I'm not sure
about all the complex switching back and forth in the SIMD codepaths. Does
__poly1305_block need to follow up with one more reduction?

I seem to be able to reproduce failures on all four of {32,64}-bit
{arm,x86}. I'm guessing the other three have similar issues, but I haven't
looked at them yet.

David

On Fri, Mar 25, 2016 at 1:25 PM Andy Polyakov via RT  wrote:

> > Attached is an updated version of the test with an additional test
> > vector. This one happens on 64 bit and not on 32 bit.
>
> Got it. It will take some time to perform cross-checks. Thanks!
>
>
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4483
> Please log in as guest with password guest if prompted
>
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4483
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 #4439] poly1305-x86.pl produces incorrect output

[David Benjamin via RT]

On Sun, Mar 20, 2016 at 10:47 PM David Benjamin  wrote:

> On Sun, Mar 20, 2016 at 5:05 PM Andy Polyakov via RT 
> wrote:
>
>> No, it doesn't depend on call pattern. Please confirm that attached
>> patch solves the problem. Thanks.
>>
>
> (Right, sorry, I meant that the test vectors I have seem to only with
> their corresponding call patterns.)
>
> The patch works on my end, and naively comparing random inputs against a
> reference implementation doesn't reveal any other issues. Thanks for fixing
> it so quickly!
>

Andy, there appears to be a typo in the patch. It says defined(extra)
rather than defined($extra). It was evaluating a bare word and always using
paddq. The $extra version seems to work too, but may I suggest adding some
comments here?

If I'm understanding correctly, the paddd vs paddq decision is about
whether the sum fits in 2^32 rather than needing the full 2^64, right? And
you use paddd preferentially over paddq because paddq is slow on Atom? This
isn't very clear from "because paddq is "broken" on Atom". It's also no
longer next to where $paddx is computed.

Moreover, it seems lazy_reduction conditioning on $extra isn't because
$extra is in itself significant, but because $extra being set means we are
following the tail logic and a horizontal addition, so the bounds don't
hold anymore? This could do with a clear comment.

Finally, where paddd is used, it's probably worth a comment for why the
bounds hold and under what assumptions. I haven't been able to trace
through them myself (based on the paper, it looks like the result of the h4
-> h0 carry after the horizontal addition should be bound by 2^26 + 2^26 *
5 * 2 * 5 = 2^26 * 51, but looking in a debugger, it's larger, so clearly
I'm missing something), so I can't suggest any particular text.

David

PS: By the way, this typo would have been caught by use strict. Have you
all considered moving perlasm to be use strict clean?

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4439
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 #4439] poly1305-x86.pl produces incorrect output

[David Benjamin via RT]

On Sun, Mar 20, 2016 at 5:05 PM Andy Polyakov via RT  wrote:

> No, it doesn't depend on call pattern. Please confirm that attached
> patch solves the problem. Thanks.
>

(Right, sorry, I meant that the test vectors I have seem to only with their
corresponding call patterns.)

The patch works on my end, and naively comparing random inputs against a
reference implementation doesn't reveal any other issues. Thanks for fixing
it so quickly!

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4439
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 #4460] [PATCH] BIO_METHODs should be const

[David Benjamin via RT]

Patch attached. This is a mechanical change. BIO_new takes a non-const
BIO_METHOD and the various BIO_METHODs defined in the library are also
non-const, so they don't get placed in .rodata.

The change to BIO_new and the BIO struct should be source-compatible.
Fixing the in-library BIO_METHODs is not. This will work as-is:
BIO *bio = BIO_new(BIO_s_mem());
This will not:
BIO_METHOD *method = BIO_s_mem();
BIO *bio = BIO_new(method);
(method would have to be const.)

If this is a concern, I can split out just the BIO_new change (so that
external BIO_METHODs may be const without requiring casts). It would be
nice to put the in-library BIOs in .rodata too, but then functions like
BIO_s_mem would need to cast away const-ness internally. Happy to switch it
to whichever is preferable.

David

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



0001-BIO_METHODs-should-be-const.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4439] poly1305-x86.pl produces incorrect output

[David Benjamin via RT]

On Thu, Mar 17, 2016 at 5:22 PM David Benjamin via RT <r...@openssl.org>
wrote:

> I'm probably going to write something to generate random inputs and stress
> all your other poly1305 codepaths against a reference implementation. I
> recommend doing the same in your own test harness, to make sure there
> aren't others of these bugs lurking around.
>

That gave a much shorter test case (or a different bug altogether?):

Key = 2d773be37adb1e4d683bf0075e79c4ee037918535a7f99ccb7040fb5f5f43aea
Input =
89dab80b7717c1db5db437860a3f70218e93e1b8f461fb677f16f35f6f87e2a91c99bc3a47ace47640cc95c345be5ecca5a3523c35cc01893af0b64a620334270372ec12482d1b1e363561698a578b359803495bb4e2ef1930b17a5190b580f141300df30adbeca28f6427a8bc1a999fd51c554a017d095d8c3e3127daf9f595
MAC = c85d15ed44c378d6b00e23064c7bcd51

This time there's no need for the funny update pattern. Feed it all into
poly1305 in one call.
$ OPENSSL_ia32cap=0 ./poly1305_test3
PASS
$ ./poly1305_test3
Poly1305 test failed.
got:  c85d15ed43c378d6b00e23064c7bcd51
expected: c85d15ed44c378d6b00e23064c7bcd51

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4439
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 #4439] poly1305-x86.pl produces incorrect output

[David Benjamin via RT]

Hi folks,

You know the drill. See the attached poly1305_test2.c.

$ OPENSSL_ia32cap=0 ./poly1305_test2
PASS
$ ./poly1305_test2
Poly1305 test failed.
got:  2637408fe03086ea73f971e3425e2820
expected: 2637408fe13086ea73f971e3425e2820

I believe this affects both the SSE2 and AVX2 code. It does seem to be
dependent on this input pattern.

This was found because a run of our SSL tests happened to find a
problematic input. I've trimmed it down to the first block where they
disagree.

I'm probably going to write something to generate random inputs and stress
all your other poly1305 codepaths against a reference implementation. I
recommend doing the same in your own test harness, to make sure there
aren't others of these bugs lurking around.

David

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

/* Place in openssl checkout (for access to poly1305.h) and build with:
 *
 * gcc -m32 ./poly1305_test2.c ./libcrypto.a -Iinclude/ -ldl -pthread -o poly1305_test2 */

#include 
#include 
#include 
#include 

#include 

#include "crypto/include/internal/poly1305.h"


/* Copy over the poly1305_context definition, so as not to fuss with sizes. */
typedef void (*poly1305_blocks_f)(void *ctx, const unsigned char *inp,
  size_t len, unsigned int padbit);
typedef void (*poly1305_emit_f)(void *ctx, unsigned char mac[16],
const unsigned int nonce[4]);
struct poly1305_context {
  double opaque[24];
  unsigned int nonce[4];
  unsigned char data[POLY1305_BLOCK_SIZE];
  size_t num;
  struct {
poly1305_blocks_f blocks;
poly1305_emit_f emit;
  } func;
};

/* Adapted from poly1305.c's SELFTEST codepath. */

static void hexdump(const uint8_t *a, size_t len) {
  size_t i;

  for (i = 0; i < len; i++) {
printf("%02x", a[i]);
  }
}

static const unsigned char kKey[32] = {
0x99, 0xe5, 0x82, 0x2d, 0xd4, 0x17, 0x3c, 0x99, 0x5e, 0x3d, 0xae,
0x0d, 0xde, 0xfb, 0x97, 0x74, 0x3f, 0xde, 0x3b, 0x08, 0x01, 0x34,
0xb3, 0x9f, 0x76, 0xe9, 0xbf, 0x8d, 0x0e, 0x88, 0xd5, 0x46,
};

static const unsigned char kInput1[] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0b,
0x17, 0x03, 0x03, 0x02, 0x00, 0x00, 0x00, 0x00,
};

static const unsigned char kInput2[] = {
0x06, 0xdb, 0x1f, 0x1f, 0x36, 0x8d, 0x69, 0x6a, 0x81, 0x0a, 0x34, 0x9c,
0x0c, 0x71, 0x4c, 0x9a, 0x5e, 0x78, 0x50, 0xc2, 0x40, 0x7d, 0x72, 0x1a,
0xcd, 0xed, 0x95, 0xe0, 0x18, 0xd7, 0xa8, 0x52, 0x66, 0xa6, 0xe1, 0x28,
0x9c, 0xdb, 0x4a, 0xeb, 0x18, 0xda, 0x5a, 0xc8, 0xa2, 0xb0, 0x02, 0x6d,
0x24, 0xa5, 0x9a, 0xd4, 0x85, 0x22, 0x7f, 0x3e, 0xae, 0xdb, 0xb2, 0xe7,
0xe3, 0x5e, 0x1c, 0x66, 0xcd, 0x60, 0xf9, 0xab, 0xf7, 0x16, 0xdc, 0xc9,
0xac, 0x42, 0x68, 0x2d, 0xd7, 0xda, 0xb2, 0x87, 0xa7, 0x02, 0x4c, 0x4e,
0xef, 0xc3, 0x21, 0xcc, 0x05, 0x74, 0xe1, 0x67, 0x93, 0xe3, 0x7c, 0xec,
0x03, 0xc5, 0xbd, 0xa4, 0x2b, 0x54, 0xc1, 0x14, 0xa8, 0x0b, 0x57, 0xaf,
0x26, 0x41, 0x6c, 0x7b, 0xe7, 0x42, 0x00, 0x5e, 0x20, 0x85, 0x5c, 0x73,
0xe2, 0x1d, 0xc8, 0xe2, 0xed, 0xc9, 0xd4, 0x35, 0xcb, 0x6f, 0x60, 0x59,
0x28, 0x00, 0x11, 0xc2, 0x70, 0xb7, 0x15, 0x70, 0x05, 0x1c, 0x1c, 0x9b,
0x30, 0x52, 0x12, 0x66, 0x20, 0xbc, 0x1e, 0x27, 0x30, 0xfa, 0x06, 0x6c,
0x7a, 0x50, 0x9d, 0x53, 0xc6, 0x0e, 0x5a, 0xe1, 0xb4, 0x0a, 0xa6, 0xe3,
0x9e, 0x49, 0x66, 0x92, 0x28, 0xc9, 0x0e, 0xec, 0xb4, 0xa5, 0x0d, 0xb3,
0x2a, 0x50, 0xbc, 0x49, 0xe9, 0x0b, 0x4f, 0x4b, 0x35, 0x9a, 0x1d, 0xfd,
0x11, 0x74, 0x9c, 0xd3, 0x86, 0x7f, 0xcf, 0x2f, 0xb7, 0xbb, 0x6c, 0xd4,
0x73, 0x8f, 0x6a, 0x4a, 0xd6, 0xf7, 0xca, 0x50, 0x58, 0xf7, 0x61, 0x88,
0x45, 0xaf, 0x9f, 0x02, 0x0f, 0x6c, 0x3b, 0x96, 0x7b, 0x8f, 0x4c, 0xd4,
0xa9, 0x1e, 0x28, 0x13, 0xb5, 0x07, 0xae, 0x66, 0xf2, 0xd3, 0x5c, 0x18,
0x28, 0x4f, 0x72, 0x92, 0x18, 0x60, 0x62, 0xe1, 0x0f, 0xd5, 0x51, 0x0d,
0x18, 0x77, 0x53, 0x51, 0xef, 0x33, 0x4e, 0x76, 0x34, 0xab, 0x47, 0x43,
0xf5, 0xb6, 0x8f, 0x49, 0xad, 0xca, 0xb3, 0x84, 0xd3, 0xfd, 0x75, 0xf7,
0x39, 0x0f, 0x40, 0x06, 0xef, 0x2a, 0x29, 0x5c, 0x8c, 0x7a, 0x07, 0x6a,
0xd5, 0x45, 0x46, 0xcd, 0x25, 0xd2, 0x10, 0x7f, 0xbe, 0x14, 0x36, 0xc8,
0x40, 0x92, 0x4a, 0xae, 0xbe, 0x5b, 0x37, 0x08, 0x93, 0xcd, 0x63, 0xd1,
0x32, 0x5b, 0x86, 0x16, 0xfc, 0x48, 0x10, 0x88, 0x6b, 0xc1, 0x52, 0xc5,
0x32, 0x21, 0xb6, 0xdf, 0x37, 0x31, 0x19, 0x39, 0x32, 0x55, 0xee, 0x72,
0xbc, 0xaa, 0x88, 0x01, 0x74, 0xf1, 0x71, 0x7f, 0x91, 0x84, 0xfa, 0x91,
0x64, 0x6f, 0x17, 0xa2, 0x4a, 0xc5, 0x5d, 0x16, 0xbf, 0xdd, 0xca, 0x95,
0x81, 0xa9, 0x2e, 0xda, 0x47, 0x92, 0x01, 0xf0, 0xed, 0xbf, 0x63, 0x36,
0x00, 0xd6, 0x06, 0x6d, 0x1a, 0xb3, 0x6d, 0x5d, 0x24, 0x15, 0xd7, 0x13,
0x51, 0xbb, 0xcd, 0x60, 0x8a, 0x25, 0x10, 0x8d, 0x25, 0x64, 0x19, 0x92,
0xc1, 0xf2, 0x6c, 0x53, 0x1c, 0xf9, 0xf9, 0x02, 0x03, 0xbc, 0x4c, 0xc1,
0x9f, 0x59, 0x27, 0xd8, 0x34, 0xb0, 0xa4, 0x71, 0x16, 0xd3, 0x88, 0x4b,
0xbb, 0x16, 

Re: [openssl-dev] [openssl.org #4362] chacha-x86.pl has stricter aliasing requirements than other files

[David Benjamin via RT]

The current state is that, as far as I can tell, overlapping requirements
are undocumented (or is it somewhere and I missed it?) and, for ChaCha,
architecture-specific. I think something certainly needs to be done. Either
changing chacha-x86.pl and allowing any out <= in overlap, or declaring
that you want out == in (or something else) with, at minimum, a
documentation change.

I would actually suggest going further and updating EVP_CipherUpdate to
enforce the rule and raise an error if the caller doesn't honor it.
Otherwise we'll continue to be in the situation where callers may write
code that works on some architectures but not others. (BoringSSL's EVP_AEAD
API will fail with OUTPUT_ALIASES_INPUT if aliasing requirements aren't
honored.)

Actually, I'm not sure how to best translate an out == in rule to streaming
EVP_CipherUpdate for block ciphers. Imagine feeding one byte at a time to
EVP_CipherUpdate, in will naturally get ahead of out and then synchronize
at block boundaries, so the rule can't be as straight forward as "out ==
in". (Whereas out <= in naturally covers this behavior.)

Given the numbers in
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005625.html
the cost seems fairly modest and this is only for 32-bit, not 64-bit. Based
on that, and that other implementations I've tested handle the case fine, I
think this is a reasonable requirement to impose.

Of course, I am also biased here because out == in will cause me some
nuisance. :-) One can certainly argue that out == in is perhaps easier to
handle than out <= in and it is not worth allowing it.

Either way, I'm not an OpenSSL team member and can't make a decision on
behalf of you all. This is something you all have to pick from.

David

On Fri, Mar 4, 2016 at 7:24 AM Andy Polyakov via RT  wrote:

> >>> If the other EVP ciphers universally allow this then I think we must
> >> treat this
> >>> as a bug, because people may be relying on this behaviour. There is
> also
> >>> sporadic documentation in lower-level APIs (AES source and des.pod)
> that
> >> the
> >>> buffers may overlap.
> >>>
> >>> If it's inconsistent then, at the very least, we must document that it
> >> is not
> >>> allowed.
> >>
> >> I'd like to argue that EVP is not place to provide any guarantees about
> >> partially overlapping buffers. Even though all current ciphers process
> >> data in ascending address order, we shouldn't make assumption that there
> >> won't be one that processes data in reverse order.
> >
> >
> > I'm afraid that, since we haven't documented it, the world may already
> have
> > made that assumption.
>
> Fear is irrational and destructive feeling. Having faith that world is
> better than that it nothing but healthy :-) What I'm saying is that
> let's put a little bit more substance into discourse. Would anybody
> consider it *sane* programming practice to rely on partially overlapping
> buffers in *general* case? I.e. without actually *knowing* (as opposite
> to *assuming*) what's gong on? [Control question: does compiler
> guarantee order of references to memory?] As said in last message I
> don't consider it sane and even consider it natural [which means that
> I'd expect majority to not consider it sane too].
>
> Once again, I'm not saying that nothing would be done, I simply want to
> figure out where does line go. From my personal view point I'd say that
> nothing *has to* be done, but it's just me. You seem to say that we're
> obliged to support partially overlapping buffers. My question then is
> *any* overlap, *any* cost? Shall we settle for simply writing down that
> application developer may not rely on partially overlapping buffers? If
> so, do we fix the modules in question arguing that this quality might be
> desirable in different context [where modules in question can be used]?
>
>
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4362
> Please log in as guest with password guest if prompted
>
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4362
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 #4401] [PATCH] plug potential memory leak(s) in OpenSSL 1.1 pre 4 in 'ec_lib.c'

[David Benjamin via RT]

By the way, returning the original subject, I don't believe there is a leak
here.

If EC_GROUP_copy fails, dest still exists and is owned by the caller. It's
the caller's obligation to call EC_GROUP_free and that will release the
partially-copied EC_GROUP. (Which will, with this patch, cause a
double-free because the unnecessarily freed pointers aren't nulled.)

David

On Wed, Mar 9, 2016 at 1:00 PM Bill Parker via RT  wrote:

> Geez,
>
> What did I start here (egad) :)
>
> Bill
>
> On Wed, Mar 9, 2016 at 5:03 AM, Salz, Rich via RT  wrote:
>
> > > No, you got that right, NULL being 'safe' to free varies with OS.
> >
> > Except we mandate ANSI C which means it's portable :)
> >
> > --
> > Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4401
> > Please log in as guest with password guest if prompted
> >
> >
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4401
> Please log in as guest with password guest if prompted
>
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4401
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 #4394] OpenSSL 1.1.0 state machine can't read handshake headers async

[David Benjamin via RT]

On Mon, Mar 7, 2016 at 5:08 PM David Benjamin via RT <r...@openssl.org> wrote:

> No patch for this one since I'm not that familiar with your state machine.
> If the peer sends handshake messages fragmented across records such that
> the handshake message header is split over two records AND the two records
> are received in different steps asynchronously, OpenSSL fails to reassemble
> the message.
>
> This is because every iteration through the READ_STATE_HEADER step in
> read_state_machine resets s->init_num.
>
> https://github.com/openssl/openssl/blob/0d4fb8439092ff8253af72ac6bc193e77ebbcf2f/ssl/statem/statem.c#L550
> Instead, it should only get reset once between messages.
>
> The Basic-Server-Async-SplitHandshakeRecords test in BoringSSL's test suite
> can be used to repro this:
> https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html
> (Also most other tests that say Async and SplitHandshakeRecords in them.)
>

Oh, I'd meant to elaborate on this test and forgot. It's part of a series
of tests that tries to stress all the async bits of the state machine.
SplitHandshakeRecords means each handshake message (I probably should have
named it SplitHandshakeMessages), gets fragmented so each record contains
only one byte. Async means we tell the shim to install a fake BIO that only
releases one byte at a time.

The combination does a good job at testing transport-related state machine
resumption points. I'm not familiar with how one writes TLSProxy tests, but
something proxy-based should also be able to simulate the handshake
fragmentation, and then async mode is implemented in the tested process.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4394
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 #4395] OpenSSL doesn't reject out-of-context empty records

[David Benjamin via RT]

ssl3_get_record silently discards empty records without much context, which
means OpenSSL will happily accept, e.g., empty app data records
mid-handshake or empty records of bogus type. They get silently discarded
and never returned to the caller, so this is harmless, just a little odd.

This is what we did to fix it:
https://boringssl.googlesource.com/boringssl.git/+/4cf369b9204f066e0ffac8fa583bd19e72c82592%5E%21/
Something similar would probably work.

The AppDataBeforeHandshake-Empty and
AppDataAfterChangeCipherSpec-Empty tests in BoringSSL's test suite can be
used to repro this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4395
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 #4394] OpenSSL 1.1.0 state machine can't read handshake headers async

[David Benjamin via RT]

No patch for this one since I'm not that familiar with your state machine.
If the peer sends handshake messages fragmented across records such that
the handshake message header is split over two records AND the two records
are received in different steps asynchronously, OpenSSL fails to reassemble
the message.

This is because every iteration through the READ_STATE_HEADER step in
read_state_machine resets s->init_num.
https://github.com/openssl/openssl/blob/0d4fb8439092ff8253af72ac6bc193e77ebbcf2f/ssl/statem/statem.c#L550
Instead, it should only get reset once between messages.

The Basic-Server-Async-SplitHandshakeRecords test in BoringSSL's test suite
can be used to repro this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html
(Also most other tests that say Async and SplitHandshakeRecords in them.)

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4394
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 #4393] [PATCH] Call EC_GROUP_order_bits in priv2opt.

[David Benjamin via RT]

The private key is a scalar and should be sized by the order, not the
degree.

(Unlike my other recent emails, this has nothing to do with BoringSSL
tests. :-) )

David

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



0007-Call-EC_GROUP_order_bits-in-priv2opt.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4392] [PATCH] Resolve DTLS cookie and version before session resumption.

[David Benjamin via RT]

Session resumption involves a version check, so version negotiation must
happen first. Currently, the DTLS implementation cannot do session
resumption in DTLS 1.0 because the ssl_version check always checks against
1.2.

Switching the order also removes the need to fixup ssl_version in DTLS
version negotiation.

The DTLS1-ECDHE-RSA-AES256-SHA-server test (and any other
DTLS1-{cipher-name}-server test) in BoringSSL's test suite can be used to
repro this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html

David

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



0006-Resolve-DTLS-cookie-and-version-before-session-resum.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4391] [PATCH] Tighten up logic around ChangeCipherSpec.

[David Benjamin via RT]

ChangeCipherSpec messages have a defined value. They also may not occur in
the middle of a handshake message. The current logic will accept a
ChangeCipherSpec with value 2. It also would accept up to three bytes of
handshake data before the ChangeCipherSpec which it would discard (because
s->init_num gets reset).

Instead, require that s->init_num is 0 when a ChangeCipherSpec comes in.

The BadChangeCipherSpec-1 test in BoringSSL's test suite can be used to
repro part of this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html
We do also have a series of FragmentAcrossChangeCipherSpec tests, but they
assume the buggy behavior was to concatenate the pre- and post-CCS
fragments, rather than drop the pre-CCS fragment. Instead, applying this
patch will repro:

diff --git a/ssl/test/runner/handshake_server.go
b/ssl/test/runner/handshake_server.go
index 0232772..3a93d48 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -907,6 +907,9 @@ func (hs *serverHandshakeState) sendFinished(out
[]byte) error {
hs.writeServerHash(hs.finishedBytes)
postCCSBytes := hs.finishedBytes

+   if !c.isDTLS {
+   c.writeRecord(recordTypeHandshake, []byte{'A'})
+   }
if c.config.Bugs.FragmentAcrossChangeCipherSpec {
c.writeRecord(recordTypeHandshake, postCCSBytes[:5])
postCCSBytes = postCCSBytes[5:]

David

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



0005-Tighten-up-logic-around-ChangeCipherSpec.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4390] [PATCH] Don't send signature algorithms when client_version is below TLS 1.2.

[David Benjamin via RT]

Per RFC 5246,

Note: this extension is not meaningful for TLS versions prior to 1.2.
Clients MUST NOT offer it if they are offering prior versions.
However, even if clients do offer it, the rules specified in [TLSEXT]
require servers to ignore extensions they do not understand.

Although second sentence would suggest that there would be no interop
problems in always offering the extension, WebRTC has reported issues with
Bouncy Castle on < TLS 1.2 ClientHellos that still include
signature_algorithms. See also
https://bugs.chromium.org/p/webrtc/issues/detail?id=4223

Just about any TLS 1.2 client test in BoringSSL's test suite can be used to
repro this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html

David

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



0004-Don-t-send-signature-algorithms-when-client_version-.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4389] [PATCH] The NewSessionTicket message is not optional.

[David Benjamin via RT]

Per RFC 4507, section 3.3:

   This message [NewSessionTicket] MUST be sent if the
   server included a SessionTicket extension in the ServerHello.  This
   message MUST NOT be sent if the server did not include a
   SessionTicket extension in the ServerHello.

The presence of the NewSessionTicket message should be determined entirely
from the ServerHello without probing.

The SkipNewSessionTicket test in BoringSSL's test suite can be used to
repro this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html

David

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



0003-The-NewSessionTicket-message-is-not-optional.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4388] [PATCH] Don't be sensitive to the order of ALPN and NPN.

[David Benjamin via RT]

If the server consumer configures NPN and not ALPN, OpenSSL should still
resolve NPN against clients which advertises it. (NB: Chrome will be
removing NPN soon, so hopefully there won't be any such consumers.) Losing
the alpn_select_cb check makes OpenSSL depend on whether ALPN or NPN comes
first in the ClientHello.

If NPN comes first, it will set next_proto_neg_seen but then the ALPN logic
will unset next_proto_neg_seen even though tls1_alpn_handle_client_hello
won't do anything. If ALPN comes first, it works.

This check used to be there, but got lost in
062178678f5374b09f00d70796f6e692e8775aca.

The NPN-Server-Sync test in BoringSSL's test suite can be used to repro
this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html
(Although I didn't actually add a test for this ordering issue explicitly.
This was found on accident because the order our Go code happened to put
ALPN and NPN in.)

David

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



0002-Don-t-be-sensitive-to-the-order-of-ALPN-and-NPN.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4387] [PATCH] Fix V2ClientHello handling

[David Benjamin via RT]

The V2ClientHello code creates an empty compression list, but the
compression list must explicitly contain the null compression (and later
code enforces this). As a result, all V2ClientHellos currently get rejected
on master.

The SendV2ClientHello-Sync test in BoringSSL's test suite can be used to
repro this:
https://mta.openssl.org/pipermail/openssl-dev/2016-March/005779.html

David

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



0001-Fix-V2ClientHello-handling.patch
Description: Binary data
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4364] [PATCH] ASN1_get_object should not accept large universal tags.

[David Benjamin via RT]

See attached. OpenSSL can't actually represent large universal tags because
it collides with the V_ASN1_NEG flag, yet it happily parses them in high
tag number form. d2i_ASN1_TYPE interprets 1f82020100 as a negative zero,
rather than an element with tag [UNIVERSAL 258].

I've intentionally made the patch very conservative, so it only limits
universal tags, in case there is worry about someone actually using tag
number 258 of another class. (Although I've never seen anything go beyond
31 into high tag number form at all.)

Our version of the change has a test:
https://boringssl.googlesource.com/boringssl/+/fb2c6f8c8565e1e2d85c24408050c96521acbcdc%5E%21/
It should be straight-forward to adapt (the test barely does anything). I'm
not sure how adding a test in OpenSSL works these days, so I leave that to
you.

David

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

>From be074e3369ee0754f1413be82d0fa0f5effc8b27 Mon Sep 17 00:00:00 2001
From: David Benjamin 
Date: Tue, 1 Mar 2016 13:58:04 -0500
Subject: [PATCH] ASN1_get_object should not accept large universal tags.

The high bits of the type get used for the V_ASN1_NEG bit, so when used with
ASN1_ANY/ASN1_TYPE, universal tags become ambiguous. This allows one to create
a negative zero, which should be impossible. Impose an upper bound on universal
tags accepted by crypto/asn1.

See also BoringSSL's
https://boringssl.googlesource.com/boringssl/+/fb2c6f8c8565e1e2d85c24408050c96521acbcdc.
---
 crypto/asn1/asn1_lib.c | 4 
 include/openssl/asn1.h | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index da1ac78..fe1473f 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -126,6 +126,10 @@ int ASN1_get_object(const unsigned char **pp, long *plength, int *ptag,
 if (--max == 0)
 goto err;
 }
+
+if (xclass == V_ASN1_UNIVERSAL && tag > V_ASN1_MAX_UNIVERSAL)
+goto err;
+
 *ptag = tag;
 *pclass = xclass;
 if (!asn1_get_length(, , plength, (int)max))
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 360914d..aa064d2 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -95,6 +95,9 @@ extern "C" {
 # define V_ASN1_ANY  -4/* used in ASN1 template code */
 
 # define V_ASN1_NEG  0x100/* negative flag */
+/* No supported universal tags may exceed this value, to avoid ambiguity with
+ * V_ASN1_NEG. */
+# define V_ASN1_MAX_UNIVERSAL0xff
 
 # define V_ASN1_UNDEF-1
 # define V_ASN1_EOC  0
-- 
2.7.0.rc3.207.g0ac5344

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

[openssl-dev] [openssl.org #4362] chacha-x86.pl has stricter aliasing requirements than other files

[David Benjamin via RT]

I'm unclear on what EVP_CIPHER's interface guarantees are, but our EVP_AEAD
APIs are documented to allow in/out buffers to alias as long as out is <=
in. This matches what callers might expect from a naive implementation.

Our AES-GCM EVP_AEADs, which share code with OpenSSL, have tended to match
this pattern too. For ChaCha, of chacha-{x86,x86_64,armv4,armv8}.pl and the
C implementation, all seem satisfy this (though it's possible I don't have
complete coverage) except for chacha-x86.pl. That one works if in == out,
but not if out is slightly behind.

We were able to reproduce problems when in = out + 1. The SSE3 code
triggers if the input is at least 256 bytes and the non-SSE3 code if the
input is at least 64 bytes. The non-SSE3 code is because the words in a
block are processed in a slightly funny order (0, 4, 8, 9, 12, 14, 1, 2, 3,
5, 6, 7, 10, 11, 13, 15). I haven't looked at the SSE3 case carefully, but
I expect it's something similar.

Could the blocks perhaps be processed in a more straight-forward ordering,
so that chacha-x86.pl behaves like the other implementations? (It's nice to
avoid bugs that only trigger in one implementation.) Or is this order
necessary for something?

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4362
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 #4346] poly1305-x86.pl's AVX2 code

[David Benjamin via RT]

On Fri, Feb 26, 2016 at 4:48 PM Andy Polyakov via RT  wrote:

> > There seems to be a bug in the AVX2 codepath in poly1305-x86.pl. I have
> not
> > attempted to debug this, but I have attached a test file which produces
> > different output in normal and AVX2 codepaths. Our existing poly1305
> > implementation agrees with the former.
> >
> > $ OPENSSL_ia32cap=0 ./poly1305_test
> > PASS
> > $ ./poly1305_test
> > Poly1305 test failed.
> > got:  2e65f0054e36505687d937ff5e8ed112
> > expected: 69d28f73dd09d39a92aa179da354b7ea
>
> While I keep looking further, double-check attached.
>

That patch makes all of my test cases pass. (Though I don't know if I have
coverage for this code because valgrind doesn't do 32-bit AVX2 yet.)

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4346
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 #4346] Re: poly1305-x86.pl's AVX2 code

[David Benjamin via RT]

On Thu, Feb 25, 2016 at 6:16 PM David Benjamin  wrote:

> From looking at valgrind, this pattern seems to give good coverage. I
> used valgrind --tool=callgrind --dump-instr=yes --collect-jumps=yes and
> then kcachegrind to inspect the output. (kcachegrind is a bit heavy for
> this. I'm hoping I can find or write a better annotator here. Something
> which looks like, say, LCOV would be ideal.)
>

For anyone else who wants to try this sort of thing, passing -g to the
assembler (so -Wa,-g) will make callgrind_annotate emit this in a useful
form without needing kcachegrind. Thanks to Steven Valdez for pointing this
out.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4346
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 #4346] poly1305-x86.pl's AVX2 code

[David Benjamin via RT]

There seems to be a bug in the AVX2 codepath in poly1305-x86.pl. I have not
attempted to debug this, but I have attached a test file which produces
different output in normal and AVX2 codepaths. Our existing poly1305
implementation agrees with the former.

$ OPENSSL_ia32cap=0 ./poly1305_test
PASS
$ ./poly1305_test
Poly1305 test failed.
got:  2e65f0054e36505687d937ff5e8ed112
expected: 69d28f73dd09d39a92aa179da354b7ea

You may wish to generalize that Poly1305_Update pattern into your own
tests. This is what I did to catch this:
https://boringssl-review.googlesource.com/#/c/7223/
>From looking at valgrind, this pattern seems to give good coverage. I
used valgrind --tool=callgrind --dump-instr=yes --collect-jumps=yes and
then kcachegrind to inspect the output. (kcachegrind is a bit heavy for
this. I'm hoping I can find or write a better annotator here. Something
which looks like, say, LCOV would be ideal.)

By the way, this assembly code is quite complicated. I wasn't able to find
problems in the others (I tested armv4, armv8, x86, and x86_64), but I'm
far from confident I've covered all the cases.

With the caveat that I'm no assembly programmer, much of the complexity
seems to come the SIMD code needing a multiple of 2 or 4 blocks and the
implementation converting internal state back and forth from base 2^26 and
2^64 and handling excess blocks slightly differently in different cases. (I
counted nine distinct codepaths to test in the x86_64 AVX codepath alone.)

The C code already buffers up to 16-byte blocks. Did you consider buffering
up to 32 or 64 bytes in C when the SIMD code called for it? I think it
could be simpler. You'd only need to handle excess blocks at the end. This
would also simplify the SIMD upgrade on long inputs, so long as the buffer
exceeds the cutoff. (You'll never process input before the upgrade.)

I haven't tried this, so perhaps the performance costs are prohibitive, but
if the costs are modest, the simplifications may be worth it.

David

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

/* Place in openssl checkout (for access to poly1305.h) and build with:
 *
 * gcc -m32 ./poly1305_test.c ./libcrypto.a -Iinclude/ -ldl -pthread -o poly1305_test */

#include 
#include 
#include 
#include 

#include 

#include "crypto/include/internal/poly1305.h"


/* Copy over the poly1305_context definition, so as not to fuss with sizes. */
typedef void (*poly1305_blocks_f)(void *ctx, const unsigned char *inp,
  size_t len, unsigned int padbit);
typedef void (*poly1305_emit_f)(void *ctx, unsigned char mac[16],
const unsigned int nonce[4]);
struct poly1305_context {
  double opaque[24];
  unsigned int nonce[4];
  unsigned char data[POLY1305_BLOCK_SIZE];
  size_t num;
  struct {
poly1305_blocks_f blocks;
poly1305_emit_f emit;
  } func;
};

/* Adapted from poly1305.c's SELFTEST codepath. */
static uint8_t hex_digit(char h) {
  if (h >= '0' && h <= '9') {
return h - '0';
  } else if (h >= 'a' && h <= 'f') {
return h - 'a' + 10;
  } else if (h >= 'A' && h <= 'F') {
return h - 'A' + 10;
  } else {
abort();
  }
}

static void hex_decode(uint8_t *out, const char *hex) {
  size_t j = 0;

  while (*hex != 0) {
uint8_t v = hex_digit(*hex++);
v <<= 4;
v |= hex_digit(*hex++);
out[j++] = v;
  }
}

static void hexdump(uint8_t *a, size_t len) {
  size_t i;

  for (i = 0; i < len; i++) {
printf("%02x", a[i]);
  }
}

static const char kKey[] =
"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";

static const char kInput[] =
"248ac31085b6c2adaaa38259a0d7192c5c35d1bb4ef39ad94c38d1c82479e2dd2159a07702"
"4b0589bc8a20101b506f0a1ad0bbab76e83a83f1b94be6beae74e874cab692c5963a75436b"
"776121ec9f62399a3e66b2d22707dae81933b6277f3c8516bcbe26dbbd86f373103d7cf4ca"
"d1888c952118fbfbd0d7b4bedc4ae4936aff91157e7aa47c54442ea78d6ac251d324a0fbe4"
"9d89cc3521b66d16e9c66a3709894e4eb0a4eedc4ae19468e66b81f271351b1d921ea55104"
"7abcc6b87a901fde7db79fa1818c11336dbc07244a40eb14cf77bde35e78ae9ad7d3f57ed7"
"e7f23926c9172f82d77684ea5ed7d74ebc6f142b997036bcb7cce8df1bbc0d5b35a46509c9"
"54fc9469d214d6238f166cbf872156b4c41d7aac5942cffb175023078252a3f36e315c5d4c"
"e0e39928a018252862becacef96a19f03bdcf46d75584299d1f8b03c0169e9e407d937145b"
"5e5024139e7022a1978f114f24cdfa23780a119735c41da8fb759bbb3f025c6ec30e6c6e9b"
"ce8615be68e392fce59fd26a8e6a6cc5c606e3848116e4d01d29565a1facfb524b6d29643b"
"826eee1e42869fc76df229dd79b39a2b1df28bb335c3a5f15a855d0121e4a6da34b5e4d5b7"
"b5d5746a03ecff70811e1516fcec1bf7462e8876a2d21710aa168c78f45a6a15015950e221"
"da85d3ec822ad6d0a6931b25a06b7bb5f3c10bb36cd4d647f9561982fde9818de5d4bf8db7"
"f86c53b4ff14928ac15f79023b61861e73e44216540bb302153770da2533de9795252ab5fb"
"77ad924c9338c8144c23d4c90dab9a18feac1a1574d4545e1435eb405e6c4c439fc724fce9"

[openssl-dev] [openssl.org #4341] [PATCH] Consistently use arm_arch.h constants in armcap assembly code.

[David Benjamin via RT]

Patch attached. This is just a little cleanup change to fix not everything
using the OPENSSL_armcap constants. (Existing ones already are using them,
so I'm assuming this is okay.)

David

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

>From a7fdfa143c2d8dbe8a738662475a033e2cf1a52f Mon Sep 17 00:00:00 2001
From: David Benjamin 
Date: Tue, 23 Feb 2016 11:41:55 -0500
Subject: [PATCH] Consistently use arm_arch.h constants in armcap assembly
 code.

Most of the assembly uses constants from arm_arch.h, but a few references to
ARMV7_NEON don't. Consistently use the macros everywhere.
---
 crypto/bn/asm/armv4-mont.pl   | 2 +-
 crypto/chacha/asm/chacha-armv4.pl | 2 +-
 crypto/poly1305/asm/poly1305-armv4.pl | 2 +-
 crypto/sha/asm/sha512-armv4.pl| 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/bn/asm/armv4-mont.pl b/crypto/bn/asm/armv4-mont.pl
index 7017ad5..6fb5bb4 100644
--- a/crypto/bn/asm/armv4-mont.pl
+++ b/crypto/bn/asm/armv4-mont.pl
@@ -121,7 +121,7 @@ bn_mul_mont:
 #ifdef	__APPLE__
 	ldr	r0,[r0]
 #endif
-	tst	r0,#1			@ NEON available?
+	tst	r0,#ARMV7_NEON		@ NEON available?
 	ldmia	sp, {r0,r2}
 	beq	.Lialu
 	add	sp,sp,#8
diff --git a/crypto/chacha/asm/chacha-armv4.pl b/crypto/chacha/asm/chacha-armv4.pl
index 55ebc9e..3e3c34c 100755
--- a/crypto/chacha/asm/chacha-armv4.pl
+++ b/crypto/chacha/asm/chacha-armv4.pl
@@ -214,7 +214,7 @@ ChaCha20_ctr32:
 # ifdef	__APPLE__
 	ldr	r4,[r4]
 # endif
-	tst	r4,#1
+	tst	r4,#ARMV7_NEON
 	bne	.LChaCha20_neon
 .Lshort:
 #endif
diff --git a/crypto/poly1305/asm/poly1305-armv4.pl b/crypto/poly1305/asm/poly1305-armv4.pl
index 2cce9df..591e530 100755
--- a/crypto/poly1305/asm/poly1305-armv4.pl
+++ b/crypto/poly1305/asm/poly1305-armv4.pl
@@ -108,7 +108,7 @@ poly1305_init:
 	and	r5,r5,r3
 
 #if	__ARM_MAX_ARCH__>=7
-	tst	r12,#1			@ check for NEON
+	tst	r12,#ARMV7_NEON		@ check for NEON
 # ifdef	__APPLE__
 	adr	r9,poly1305_blocks_neon
 	adr	r11,poly1305_blocks
diff --git a/crypto/sha/asm/sha512-armv4.pl b/crypto/sha/asm/sha512-armv4.pl
index 56bb9fd..9e3c4db 100644
--- a/crypto/sha/asm/sha512-armv4.pl
+++ b/crypto/sha/asm/sha512-armv4.pl
@@ -287,7 +287,7 @@ sha512_block_data_order:
 #ifdef	__APPLE__
 	ldr	r12,[r12]
 #endif
-	tst	r12,#1
+	tst	r12,#ARMV7_NEON
 	bne	.LNEON
 #endif
 	add	$len,$inp,$len,lsl#7	@ len to point at the end of inp
-- 
2.7.0.rc3.207.g0ac5344

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

Re: [openssl-dev] [openssl.org #4323] chacha-armv4.pl bugs

[David Benjamin via RT]

On Sun, Feb 21, 2016 at 3:27 PM Andy Polyakov via RT  wrote:

> Hi,
>
> > The partial-block tail code in chacha-armv4.pl also seems to have
> problems.
> > My colleague Steven and I made an attempt to debug it, but we're not
> > familiar enough with ARM to fix it.
> >
> > From playing with it in a debugger, it doesn't look like @t[3] contains
> the
> > length. We suspect something is going wrong with the condition flags on
> > loading or updating length.
> >
> https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/chacha/asm/chacha-armv4.pl;h=55ebc9e586475a35e313b74483eb4b8d5b6f2b03;hb=HEAD#l585
>
> Attached is patch for chacha-armv4.pl (please verify) and a test snippet
> I've put together.
>

The fix seems to work. And it's a decent bit faster than our current NEON
code too. :-) Thanks!


> > It may be worth going back and testing these cases on all of the
> > implementations as well.
>
> Besides armv4 only s390x module was failing.
>

Can confirm that the armv8 code is fine.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4323
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 #4323] chacha-armv4.pl bugs

[David Benjamin via RT]

Hi Andy,

The partial-block tail code in chacha-armv4.pl also seems to have problems.
My colleague Steven and I made an attempt to debug it, but we're not
familiar enough with ARM to fix it.

>From playing with it in a debugger, it doesn't look like @t[3] contains the
length. We suspect something is going wrong with the condition flags on
loading or updating length.
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/chacha/asm/chacha-armv4.pl;h=55ebc9e586475a35e313b74483eb4b8d5b6f2b03;hb=HEAD#l585

It may be worth going back and testing these cases on all of the
implementations as well.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4323
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 #4305] ChaCha20 assembly bugs

[David Benjamin via RT]

Hi folks,

I've started playing with the ChaCha20 assembly that was recently checked
in and found a few problems. Most of these do not affect OpenSSL as you
only ever call ChaCha20_ctr32 on a whole number of blocks. But this isn't
documented as a constraint in internal/chacha.h and the assembly has code
for partial blocks, so it seems it was supposed to work. (If not, I'd
recommend removing the codepaths and documenting the constraint.)

1. In chacha-x86_64.pl, .Ltail:
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/chacha/asm/chacha-x86_64.pl;h=41dbef51b26db07a78d8939c728a8da5c703d806;hb=HEAD#l345
the xor %rbx,%rbx line clobbers @x[1] right before it is read. (@x[1] is
%rbx.) It should be moved one line down or a different register used.

2. In chacha-x86_64.pl, .Loop_tail_ssse3:
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/chacha/asm/chacha-x86_64.pl;h=41dbef51b26db07a78d8939c728a8da5c703d806;hb=HEAD#l522
The length decrement loop is wrong and instead counts up from 0 to 2^64. It
also clobbers $len because $len is %rdx. This seems to work instead:
.Loop_tail_ssse3:
movzb   ($inp,%rbx),%eax
movzb   (%rsp,%rbx),%ecx
lea 1(%rbx),%rbx
xor %ecx,%eax
mov %al,-1($out,%rbx)
dec $len
jnz .Loop_tail_ssse3

3. In chacha-x86.pl, loop:
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/chacha/asm/chacha-x86.pl;h=60d604882f76c227798895da6fafd798834f467a;hb=HEAD#l207
The line:
 ($b,(3));# load len
should say:
 ($b,(2));# load len
wparam(3) is the pointer to the key. This works in OpenSSL's calls because
pointers are typically larger than 64, and that's sufficient for the
codepaths you exercise.

4. The assembly versions crash if you pass in an empty input/output. The
generic C code handles this fine. (I'll defer to you whether this is a bug
or a caller obligation to be documented.)

I have not tested the AVX2 or XOP code yet. I'll let you know if I find
problems.

David

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4305
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 #4278] DH_CHECK_PUBKEY_INVALID should be 0x4, not 0x3

[David Benjamin via RT]

The recently-added DH_CHECK_PUBKEY_INVALID was set to 0x3, but
DH_CHECK_PUBKEY_* values are flags, so it should be 0x4 to avoid colliding
with DH_CHECK_PUBKEY_TOO_SMALL (0x01) and DH_CHECK_PUBKEY_TOO_LARGE (0x02).
See DH_check_pub_key's *ret |= logic.

https://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=b128abc3437600c3143cb2145185ab87ba3156a2


(Also, that code is missing malloc failure checks on BN_set_word,
BN_sub_word, and BN_copy. Though I could believe the first two don't
actually end up calling malloc; I didn't check.)

David

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

[openssl-dev] [openssl.org #4277] DSAPublicKey should use dsa_cb in 1.1.0

[David Benjamin via RT]

DSAPublicKey lost the dsa_cb in
https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=ea6b07b54c1f8fc2275a121cdda071e2df7bd6c1

This results in d2i_DSAPublicKey using crypto/asn1's default allocation
logic rather than calling into DSA_new. I believe it should
use ASN1_SEQUENCE_cb.

I've attached a tiny sample program. On my machine, when building against
master, the second reference count is 0 and then the second DSA_free
crashes.

Also, the comment in dsa_asn1.c and d2i_DSAPublicKey.pod both still refer
to write_params which no longer exists.

David



dsa_public_key.c
Description: Binary data
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4185] Bug in EVP_MD_CTX_copy_ex's malloc failure handling

[David Benjamin via RT]

On Thu, Dec 17, 2015 at 2:43 PM Kurt Roeckx via RT <r...@openssl.org> wrote:

> On Wed, Dec 16, 2015 at 11:34:56PM +0000, David Benjamin via RT wrote:
> > EVP_MD_CTX_copy_ex is implemented with memcpy, followed by manually
> fixing
> > up |out->pctx| and |out->md_data|.
> >
> >
> https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/evp/digest.c;h=5da0e01039a6da039942db9f1bf8b70753f509f2;hb=HEAD#l292
> >
> > If allocating |out->md_data| fails, then both |out->pctx| and |in->pctx|
> > may point to the same EVP_PKEY_CTX and freeing |out| will cause problems.
> >
> > We fixed this by not using memcpy.
> >
> https://boringssl.googlesource.com/boringssl/+/306ece31bcaaed49e0240a2ef8901ebb2d45%5E%21/crypto/digest/digest.c
>
> This patch won't apply as is since we have more fields (engine,
> flags).
>
> We also don't have pctx_ops, but have update instead, but already
> seem to copy that anyway.
>

Right, we've diverged enough at this point that patches not applying as-is
is the norm. :-) That was meant as a reference, but someone will need to do
the equivalent change in OpenSSL if you all like the approach.

David

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

[openssl-dev] [openssl.org #4184] Memory leak in DSA redo case

[David Benjamin via RT]

dsa_do_sign retries the operation if |r| or |s| end up zero. This results
in leaking the first iteration's value of |ret| since you end up clobbering
the previous allocation.
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/dsa/dsa_ossl.c;h=34b4a4ea4a267b62b21916a85ab79350cd276065;hb=HEAD#l135

The fix is to switch the order of the check and allocating |ret|:
See
https://boringssl.googlesource.com/boringssl/+/2936170d68ec617e1e6f0c2def86728ba29312b7%5E%21/#F0

(This was found via clang's scan-build tool.)

David

___
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 #4185] Bug in EVP_MD_CTX_copy_ex's malloc failure handling

[David Benjamin via RT]

EVP_MD_CTX_copy_ex is implemented with memcpy, followed by manually fixing
up |out->pctx| and |out->md_data|.

https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/evp/digest.c;h=5da0e01039a6da039942db9f1bf8b70753f509f2;hb=HEAD#l292

If allocating |out->md_data| fails, then both |out->pctx| and |in->pctx|
may point to the same EVP_PKEY_CTX and freeing |out| will cause problems.

We fixed this by not using memcpy.
https://boringssl.googlesource.com/boringssl/+/306ece31bcaaed49e0240a2ef8901ebb2d45%5E%21/crypto/digest/digest.c

David

___
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 #4119] DTLS resets handshake hash too frequently for ClientHello

[David Benjamin via RT]

On Wed, Nov 4, 2015 at 7:04 AM Matt Caswell via RT <r...@openssl.org> wrote:

>
>
> On 03/11/15 17:43, David Benjamin via RT wrote:
>
> > I'm not sure that fix quite works though. If BIO_flush completes
> > asynchronously
>
> Ahhh, yes good point. Updated patch attached.
>
> > (hrm, it's missing an rwstate update),
>
> Yes, just discovered that myself and then came back and reread your
> email to find out you already pointed it out! Also addressed in updated
> patch.
>

The new patch seems to almost work. I merged it into our codebase since we
hadn't diverged too much on that function yet and ran it against our tests
(fixed to actually stress low MTUs). The s->init_off <=
DTLS1_HM_HEADER_LENGTH assertion is only true in the frag_off > 0 case.
After moving it there, everything passes.

For reference, here's the merged version:
https://boringssl-review.googlesource.com/#/c/6424/

David

PS: By the way, you might want to consider doing something similar to test
all the resume points, since they're apparently pretty subtle and hard to
get right sometimes. :-)

I added an async mode to our tests which installs a filter BIO that only
lets one byte/datagram through at a time.
https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/async_bio.cc
It also makes every callback take two steps where possible.
https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/bssl_shim.cc#510

That's driven like this:
https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/bssl_shim.cc#802
https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/bssl_shim.cc#873

Then that gets run through a series of tests intended to cover all the
various possible handshake shapes.
https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/runner/runner.go#2539

It's worked pretty well, assuming I remember to cover all the interesting
cases. (Apparently I missed one this time around, so it's not perfect. :-)
Still, it's caught a lot of mistakes early.)

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

Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[David Benjamin via RT]

On Tue, Nov 3, 2015 at 11:16 AM Matt Caswell via RT  wrote:

> Whilst investigating this I noticed another bug which is actually
> probably more significant. My eyeball only look at the BoringSSL source
> suggests that it is there too, so I'm not sure why you haven't seen it
> in the test that you mentioned.
>
> If a retry occurs on a second or subsequent fragment of a handshake
> message then when we do the retry the wrong fragment offset and length
> is used. The impact isn't that great because the messages got dropped by
> the peer, and then when they get retransmitted they have the correct
> values inserted...so the handshake succeeds - but it gets delayed.
> Perhaps that is why you don't see it in your test.
>

Our tests are pretty strict and deterministic (clock is mocked out, Go DTLS
half is intentionally much stricter than a real impl), so it actually fails
if packets are dropped that aren't supposed to be. I think it's actually
because my async tests, uh, didn't stress the low MTU case at all. :-)
Thanks!

(The handshake hash bug requires a fragmented ClientHello to trigger in
OpenSSL because you only update the handshake hash as it gets written,
whereas BoringSSL updates it up-front to simplify a few things. I actually
didn't think much about fragmentation when working on it our end and only
realized it was a precondition for you later.)

In fact, apparently I'd noticed frag_off was messed up before, added TODOs,
and forgot about it. :-) I guess this must have been the other reason I'd
previously assumed retrying writes in the DTLS code didn't work at all. It
mostly doesn't.
https://boringssl.googlesource.com/boringssl/+/master/ssl/d1_both.c#285

I'm not sure that fix quite works though. If BIO_flush completes
asynchronously (hrm, it's missing an rwstate update), then I believe you'll
be in a state where you *do* want to repeat the init_off / init_num adjust.
You might be able to get away with using init_off/init_num with some minor
tweaks? Another problem: because the fragment headers clobber whatever was
already written, msg_callback sees garbage. Also this function is used for
the unfragmented ChangeCipherSpec, so it's even messier.

I dunno, this code is too stateful by several orders of magnitude. I think
I'm going to toy with rewriting it now rather than think too hard about the
existing mess.


> This second issue occurs in all branches.
>
> One other related point is that fragmenting ClientHellos is probably a
> bad idea. The whole ClientHello/HelloVerifyRequest mechanism is meant to
> be implemented without storing state on the server. That isn't possible
> if you have to deal with fragment reassembly. In the new DTLSv1_listen
> implementation in master we drop fragmented ClientHellos.
>

Yeah, this part of DTLS (like much of it) is woefully underspecified. We
keep stuffing things into ClientHellos, so if one does need to support
fragmented ones, I think the right way to do stateless HelloVerifyRequest
is to silently drop all non-initial ClientHello fragments and require the
initial one be large enough to include the client_random and whatever else
you figure into the cookie.

David

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

Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[David Benjamin via RT]

On Tue, Nov 3, 2015 at 12:42 PM David Benjamin  wrote:

> I'm not sure that fix quite works though. If BIO_flush completes
> asynchronously (hrm, it's missing an rwstate update), then I believe you'll
> be in a state where you *do* want to repeat the init_off / init_num adjust.
> You might be able to get away with using init_off/init_num with some minor
> tweaks? Another problem: because the fragment headers clobber whatever was
> already written, msg_callback sees garbage. Also this function is used for
> the unfragmented ChangeCipherSpec, so it's even messier.
>
> I dunno, this code is too stateful by several orders of magnitude. I think
> I'm going to toy with rewriting it now rather than think too hard about the
> existing mess.
>

This still needs to be reviewed, but here's a go at a cleaner version on
our end. It passes our test suite, even after modifying it to stress the
async write + low MTU case. (And the old code indeed does not.)

https://boringssl-review.googlesource.com/#/c/6420/
https://boringssl-review.googlesource.com/#/c/6421/

David

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

[openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[David Benjamin via RT]

Hey folks,

We found a small DTLS bug while writing some tests. I think it affects
1.0.1 and 1.0.2 too, so I thought I'd send you a note. (Note sure about
master. I'm unfamiliar with the new state machine mechanism.)

In DTLS, each ClientHello is supposed to reset the handshake hash (in case
of HelloVerifyRequest). The old state machine calls ssl3_init_finished_mac
in the SSL3_ST_CW_CLNT_HELLO_* states.

https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=ssl/d1_clnt.c;h=feeaf6d0656f5d0868121852d42b5037b8823111;hb=refs/heads/OpenSSL_1_0_2-stable#l317

If the ClientHello is fragmented and takes multiple iterations to write,
the state machine is re-entered in SSL3_ST_CW_CLNT_HELLO_B, which
calls ssl3_init_finished_mac again, and clobbers what was sampled to the
handshake hash/buffer previously.

This requires the transport return a retriable error on write, which
probably isn't common for DTLS. It came up because WebRTC uses a custom BIO
with a fixed-size buffer, so it can actually do this. Even then, a
ClientHello is unlikely to fill up the buffer. Our tests repro'd it in
BoringSSL by forcing every write to take two iterations.

David

___
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 #4120] CertificateStatus message is optional

[David Benjamin via RT]

It seems unlikely I'll be getting around to doing another newsletter, but
while I'm reporting bugs, here's another that came to mind:

RFC 6066 is somewhat obnoxious and allows the server to decline to send
CertificateStatus even after negotiating the extension.

https://tools.ietf.org/html/rfc6066#section-8
   Note that a server MAY also choose not to send a "CertificateStatus"
   message, even if has received a "status_request" extension in the
   client hello message and has sent a "status_request" extension in the
   server hello message.

OpenSSL fails the handshake when the message is omitted. We had a report of
an incompatibility because of this. I think it turned out IIS would
negotiate certificate_status, send the ServerHello, and later try to find
an OCSP response to staple. If it failed to do so, it would send no
CertificateStatus.

See also: https://crbug.com/478947

David

___
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