WIP: Harmonize calls to EVP_Digest*{Init,Update,Final}() by mspncp · Pull Request #6854 · openssl/openssl

mspncp · 2018-08-03T23:18:05Z

Commit 2 was motivated by @Bugcheckers remark #6819 (comment). The other two commits are by-products.

Commit 1

    EVP_DigestInit.pod: add missing entries to RETURN VALUES section

Commit 2

    Harmonize calls to EVP_Digest*Update()
    
    The majority of callers does not check for an empty buffer (buffer_length == 0) before calling
    EVP_Digest*Update(), because all implementations handle this case correctly as no-op.
    (See discussions in #6800, #6817, and #6838.)

Commit 3

Noticed while working on commit 2. Can be squashed with commit 2 if desired, or discarded if you think it is consistency overkill.

    Harmonize calls to EVP_Digest*{Init,Update,Final}()
    
    According to the manual pages, the functions return a boolean value,
    so checking for `EVP_Digest*() <= 0` is misleading. Also the
    statistics show a clear tendency towards using `!EVP_*()`
    
    ~/src/openssl$ grep -rn 'EVP_Digest\(Sign\)\?\(Init\|Update\|Final\).*<=' | wc -l
    77
    ~/src/openssl$ grep -rn '!EVP_Digest\(Sign\)\?\(Init\|Update\|Final\)' | wc -l
    245

mspncp · 2018-08-03T23:20:49Z

crypto/evp/m_sigver.c

Checking for sigret != NULL looks like a mistake.

No. If sigret == NULL, then it's expected that the caller tries to figure out what the signature length will be. In that case, the EVP_DigestSignUpdate call is a waste of cycles (and I have no idea what corner cases it might hit as well)

(or at least that's my guess. It might be prudent to look through history on this one)

kroeckx · 2018-08-04T03:09:53Z

At least this test fails:

        not ok 1 - ../../../_srcdist/test/recipes/30-test_evp_data/evppkey.txt
    not ok 1 - run_file_tests
../../util/shlib_wrap.sh ../../test/evp_test ../../../_srcdist/test/recipes/30-test_evp_data/evppkey.txt => 1
not ok 7 - running evp_test evppkey.txt

mspncp · 2018-08-04T06:07:15Z

Thanks Richard! The latest fixup seems heal most of the evp_test, except for the SM2 tests, which I will have to look at later. Strangely, the test outcome is 'ok' although the SM2 tests report a mismatch. Is this a bug in the test?

    # Starting "SM2 tests" tests at line 18402
    # ERROR: (memory) 'expected->output == got' failed @ test/evp_test.c:1111
    # --- expected->output
    # +++ got
    # 0000:-3045022100f11bf3 6e75bb304f094fb4 2a4ca22377d0cc76 8637c5011cd59fb9
    # 0000:+3045022061fa912d d72fff1aebf4857a 77189bdf25c2ae63 fe6d530e7878d89d
    #             ^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
    # 0020:-ed4b130c98022035 545ffe2c2efb3abe e4fee661468946d8 86004fae8ea53115
    # 0020:+570505b602210099 be0398f934a02cc6 d47cf774e44294ad b35c6741ad81693c
    #       ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
    # 0040:-93e48f7fe21b91
    # 0040:+a14caef35d5af2
    #       ^^^^^^^^^^^^^^
    #
    # INFO:  @ test/testutil/stanza.c:121
    # Starting "Chosen Wycheproof vectors" tests at line 18440
    # INFO:  @ test/testutil/stanza.c:33
    # Completed 1442 tests with 0 errors and 0 skipped
    ok 1 - test/recipes/30-test_evp_data/evppkey.txt
ok 1 - run_file_tests

mspncp · 2018-08-04T07:30:10Z

I created issue #6857 for the test problem.

The majority of callers does not check for an empty buffer (buffer_length == 0) before calling EVP_Digest*Update(), because all implementations handle this case correctly as no-op. (See discussions in openssl#6800, openssl#6817, and openssl#6838.)

[extended tests] According to the manual pages, the functions return a boolean value, so checking for `EVP_Digest*() <= 0` is misleading. Also the statistics show a clear tendency towards using `!EVP_Digest*()` (77 vs. 245 locations).

mspncp · 2018-08-04T19:27:44Z

Since @romen and @kroeckx pointed out to me that the SM2 test was not really failing, I think this pull request is ready for review now. I rebased, squashed the fixup and reordered the commits but did not make any additional source code changes. Only a little rewording of the commit messages.

levitte · 2018-08-06T11:13:13Z

ssl/ssl_lib.c

    ssl_clear_hash_ctx(hash);
    *hash = EVP_MD_CTX_new();
-    if (*hash == NULL || (md && EVP_DigestInit_ex(*hash, md, NULL) <= 0)) {
+    if (*hash == NULL || (md && !EVP_DigestInit_ex(*hash, md, NULL))) {


Hmmm... Do we call this function with a NULL md, ever? Is it reasonable to expect it to be NULL? If not, then I don't see why we're testing it...

Hmmm, good question... Its seems to be the only place in the entire source code where this is checked.

~/src/openssl$ rgrep EVP_DigestInit | grep -F '&&' ssl/ssl_lib.c:4396: if (*hash == NULL || (md && !EVP_DigestInit_ex(*hash, md, NULL))) {

Probably you're right. I will remove the check and see what the tests say about it.

Just noticed right now that the doc comment says that the md is optional. So even if nobody would use this feature we still couldn't change it in this release.

mspncp · 2018-08-08T12:14:37Z

ping

mspncp · 2018-08-13T03:13:37Z

ping @openssl/omc for a second review.

levitte · 2018-08-13T05:01:44Z

Speaking of harmonization, have you read doc/man3/EVP_DigestSignInit.pod? Take an extra look at the "RETURN VALUES" section. That would need an update, no?

mspncp · 2018-08-13T06:06:38Z

Hmmm, ok... thanks for pointing out that inconsistency with the documentation. I will have to check the sources first whether the documentation is correct and EVP_DigestSign*() and EVP_Digest*() indeed return different kind of values. The EVP_DigestSign*() call statistics is undecided; either way, approximately half of the calls could be harmonized.

EVP_DigestSign

~/src/openssl$ grep -rn 'EVP_DigestSign\(Init\|Update\|Final\).*<=' | wc -l
26
~/src/openssl$ grep -rn '!EVP_DigestSign\(Init\|Update\|Final\)' | wc -l
20

EVP_Digest

~/src/openssl$ grep -rn 'EVP_Digest\(Init\|Update\|Final\).*<=' | wc -l
51
~/src/openssl$ grep -rn '!EVP_Digest\(Init\|Update\|Final\)' | wc -l
225

I'll mark it WIP because there is still some work to be done and I did not have much time to do it recently.

mspncp · 2019-07-17T21:49:15Z

This pull request has aged and I currently have no plans to resume it.

mspncp mentioned this pull request Aug 3, 2018

Add a zero length check to EVP_DigestUpdate. #6819

Closed

mspncp commented Aug 3, 2018

View reviewed changes

mspncp mentioned this pull request Aug 4, 2018

tests: evp_test succeeds after "SM2 tests" report a fake error #6857

Closed

mspncp added 3 commits August 4, 2018 21:13

Harmonize calls to EVP_Digest*Update()

80a72e5

The majority of callers does not check for an empty buffer (buffer_length == 0) before calling EVP_Digest*Update(), because all implementations handle this case correctly as no-op. (See discussions in openssl#6800, openssl#6817, and openssl#6838.)

EVP_DigestInit.pod: add missing entries to RETURN VALUES section

45ad046

Harmonize calls to EVP_Digest*{Init,Update,Final}()

226e65c

[extended tests] According to the manual pages, the functions return a boolean value, so checking for `EVP_Digest*() <= 0` is misleading. Also the statistics show a clear tendency towards using `!EVP_Digest*()` (77 vs. 245 locations).

mspncp force-pushed the pr-harmonize-calls-to-evp-digest-functions branch from 2c826cb to 226e65c Compare August 4, 2018 19:19

paulidale previously approved these changes Aug 6, 2018

View reviewed changes

dot-asm added the approval: review pending This pull request needs review by a committer label Aug 6, 2018

levitte reviewed Aug 6, 2018

View reviewed changes

mspncp removed the approval: review pending This pull request needs review by a committer label Aug 18, 2018

mspncp changed the title ~~Harmonize calls to EVP_Digest*{Init,Update,Final}()~~ WIP: Harmonize calls to EVP_Digest*{Init,Update,Final}() Aug 18, 2018

mattcaswell added this to the Post 1.1.1 milestone Sep 3, 2018

mspncp closed this Jul 17, 2019

mspncp deleted the pr-harmonize-calls-to-evp-digest-functions branch November 25, 2019 19:30

Uh oh!

Comments

Conversation

mspncp commented Aug 3, 2018

Commit 1

Commit 2

Commit 3

Uh oh!

mspncp Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

levitte Aug 4, 2018

Choose a reason for hiding this comment

Uh oh!

levitte Aug 4, 2018

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Aug 4, 2018

Uh oh!

mspncp commented Aug 4, 2018

Uh oh!

mspncp commented Aug 4, 2018

Uh oh!

mspncp commented Aug 4, 2018

Uh oh!

levitte Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

mspncp Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mspncp Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mspncp commented Aug 8, 2018

Uh oh!

mspncp commented Aug 13, 2018

Uh oh!

levitte commented Aug 13, 2018

Uh oh!

mspncp commented Aug 13, 2018

Uh oh!

mspncp commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mspncp Aug 6, 2018 •

edited

Loading

mspncp Aug 6, 2018 •

edited

Loading