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

param_bld: add a padded BN call. #10840

Closed
wants to merge 1 commit into from

Conversation

paulidale
Copy link
Contributor

To aviod leaking size information when passing private value using the
OSSL_PARAM builder, a padded BN call is required.

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Jan 14, 2020
@paulidale paulidale added this to In progress in 3.0 New Core + FIPS via automation Jan 14, 2020
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual might need a teeny bit more

doc/internal/man3/ossl_param_bld_init.pod Outdated Show resolved Hide resolved
3.0 New Core + FIPS automation moved this from In progress to Needs review Jan 14, 2020
crypto/param_build.c Outdated Show resolved Hide resolved
@slontis
Copy link
Member

slontis commented Jan 14, 2020

make update required..

@paulidale paulidale removed the approval: review pending This pull request needs review by a committer label Jan 14, 2020
@paulidale
Copy link
Contributor Author

Feedback addressed.

@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Jan 14, 2020
@paulidale
Copy link
Contributor Author

Ping reviewer.

@richsalz
Copy link
Contributor

I wish there were a resolution to #10840 (comment) :(

@paulidale
Copy link
Contributor Author

I think we disagree.
We'll see what any reviewers say -- it's an = sign change to the code.

@levitte
Copy link
Member

levitte commented Jan 17, 2020

I wish there were a resolution to #10840 (comment) :(

There's nothing to resolve. That usage example is just that, but does not affect this PR per se.
(and I agree that it wasn't an example of passing a BIGNUM with padding, it's an example of passing a BIGNUM without)

@levitte
Copy link
Member

levitte commented Jan 17, 2020

I expected to see this rewrite of ossl_param_bld_push_BN(OSSL_PARAM_BLD:

int ossl_param_bld_push_BN(OSSL_PARAM_BLD *bld, const char *key,
                           const BIGNUM *bn)
{
    return ossl_param_bld_push_BN_pad(bld, key, bn, bn == NULL ? 0 : BN_num_bytes(bn));
}

UPDATE to handle NULL bn

@paulidale
Copy link
Contributor Author

That rewrite isn't quite right. A NULL bn argument is permitted by that function.

@levitte
Copy link
Member

levitte commented Jan 17, 2020

Er, what? Looking at the code, they seem to treat a NULL bn exactly the same way...

@levitte
Copy link
Member

levitte commented Jan 17, 2020

Sorry, now I see. I've update my proposed rewrite

@paulidale
Copy link
Contributor Author

I've made the change.

3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Jan 17, 2020
@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 17, 2020
@levitte
Copy link
Member

levitte commented Jan 17, 2020

Please check the CIs before merging

To aviod leaking size information when passing private value using the
OSSL_PARAM builder, a padded BN call is required.
@paulidale paulidale removed the approval: done This pull request has the required number of approvals label Jan 19, 2020
@paulidale paulidale added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jan 19, 2020
@paulidale
Copy link
Contributor Author

Merged to master. Thanks for the input.

@paulidale paulidale closed this Jan 19, 2020
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Jan 19, 2020
@paulidale paulidale deleted the param-bld-bn-pad branch January 19, 2020 00:20
openssl-machine pushed a commit that referenced this pull request Jan 19, 2020
To aviod leaking size information when passing private value using the
OSSL_PARAM builder, a padded BN call is required.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #10840)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants