Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Directly return from final sha3/keccak_final if no bytes are requested #9433

Closed
wants to merge 2 commits into from

Conversation

@p-steuer
Copy link
Member

commented Jul 22, 2019

Reference implementation in keccak1600.c does not touch output buffer
if output lenght is zero while some asm implementations do.

A regression test for this issue is added to evp_test.c

Fixes #9431

I need help to verify the fix for architectures i have no access.

Already verified for: s390.

  • documentation is added or updated
  • tests are added or updated
@p-steuer p-steuer force-pushed the p-steuer:sha3 branch 2 times, most recently from 4d312c5 to f42e00a Jul 22, 2019
@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Let me know if I should this this on some platforms. I have access to all Debian architectures.

@p-steuer p-steuer force-pushed the p-steuer:sha3 branch 3 times, most recently from 6ecfe18 to 9411d62 Jul 22, 2019
@t8m

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I suppose all the assembler code implementations have this deficiency.

@p-steuer p-steuer force-pushed the p-steuer:sha3 branch from f791e89 to 26337a0 Jul 23, 2019
@p-steuer

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

no fix needed, verified:

  • keccak1600-x86_64.pl

fixed, verified:

  • keccak1600-s390x.pl
  • keccak1600-ppc64.pl
  • keccak1600p8-pcc.pl
  • keccak1600-x86_64.pl
  • keccak1600-avx2.pl

no fix needed, not verified:

  • keccak1600-mmx.pl

fixed, not verified:

  • keccak1600-avx512.pl
  • keccak1600-avx512vl.pl

todo:

  • keccak1600-armv4.pl
  • keccak1600-armv8.pl
  • keccak1600-c64x.pl

Please hep with the still to be verified platforms, if you have access or an emulation set up.

I dont feel comfortable making changes to the arm and c64 asm. @dot-asm could you help here ?

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Isn't this easier to fix in the C code, for all platforms at the same time?

@p-steuer

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

certainly easier but is it the right thing to do ? i think the other asm modules handle len=0 input. opinions ?

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I would do the check as early as possible.

@dot-asm

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Here is "philosophical" question. Where would you stop? I mean if you test for buffer length, then why not test for pointers' validity[!] as well? And block size? What I'm trying to say is that there is a reasonable limit for amount of checks, and it is not inappropriate to make it caller's responsibility. Observation is that some assembly subroutines behave differently from reference implementation. So what? One can legitimately claim that despite that reference SHA3_squeeze implementation does nothing if zero length is passed, doing so constitutes undefined behaviour. It won't be a contradiction.

Also, keep in mind that this is private interface, application code won't be making the direct call. So question is where does length argument come from? It's either constant or value set with EVP_MD_CTRL_XOF_LEN. But then wouldn't it be more appropriate to vet the value in shake_ctrl? Because that's where you can communicate to application whether or not it's suitable? As opposite to choosing to opt for something application has no way of figuring out as effectively suggested here?

Just in case, as for vetting value in shake_ctrl. Note that it's int that is assigned to size_t. So that non-zero check wouldn't be right, one has to check for positivity.

But in either case, let's say that consensus would still be to check for non-zero length in all SHA3_squeeze. Since we don't check for pointers' validity, would it be appropriate to assume that pointer to A[][] is always valid? Why question? Well, it's only appropriate to aim for minimizing amount of branches. This in turn means that if a branch has to be added, it would be appropriate to position it some place where it would be less likely to be evaluated. In this case to squeeze_tail, which is actually an [highly] unlike path. This in turn would mean that for example on PPC A[][] would be always dereferenced... What I'm getting at is that it's possible to avoid additional misprediction penalty for most common scenario, which should be viewed as desirable goal.

p-steuer added 2 commits Aug 5, 2019
Requesting zero bytes from shake previously led to out-of-bounds write
on some platforms.

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>
Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>
@p-steuer p-steuer force-pushed the p-steuer:sha3 branch from 26337a0 to 0281ecf Aug 5, 2019
@p-steuer

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

after comments from @kroeckx and @dot-asm , i reworked this one:

  • passing zero length to SHA3_squeeze is now undefined behavior
  • the zero length check is moved to sha3/keccak_final
@p-steuer p-steuer changed the title WIP: Match the SHA3_squeeze asm implementations to reference implementation Directly return from final sha3/keccak_final if no bytes are requested Aug 5, 2019
@p-steuer

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

ping. after the change i think i can remove the test code: we dont test for out-of-bounds writes in other places and this can be left to address sanitizers ..

@@ -404,6 +404,28 @@ static int digest_test_run(EVP_TEST *t)
}

if (EVP_MD_flags(expected->digest) & EVP_MD_FLAG_XOF) {
EVP_MD_CTX *mctx_cpy;
char dont[] = "touch";

This comment has been minimized.

Copy link
@levitte

levitte Aug 17, 2019

Member

This made me giggle :-)

@p-steuer p-steuer self-assigned this Aug 18, 2019
levitte pushed a commit that referenced this pull request Aug 18, 2019
Requesting zero bytes from shake previously led to out-of-bounds write
on some platforms.

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9433)
levitte pushed a commit that referenced this pull request Aug 18, 2019
Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9433)
levitte pushed a commit that referenced this pull request Aug 18, 2019
Requesting zero bytes from shake previously led to out-of-bounds write
on some platforms.

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9433)

(cherry picked from commit a890ef8)
levitte pushed a commit that referenced this pull request Aug 18, 2019
Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9433)

(cherry picked from commit 3ce4643)
@p-steuer

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

pushed to master: a890ef8 3ce4643
cherry-picked to 1.1.1: 6087d4a efc62e6

@p-steuer p-steuer closed this Aug 18, 2019
@p-steuer p-steuer deleted the p-steuer:sha3 branch Aug 18, 2019
tniessen added a commit to tniessen/node that referenced this pull request Oct 5, 2019
This guard used to prevent segfaults caused by a bug in OpenSSL, but
this was fixed in OpenSSL 1.1.1d.

Refs: openssl/openssl#9433
Refs: openssl/openssl#9431
@tniessen tniessen referenced this pull request Oct 5, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.