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

Ensures that EVP encryption & decryption operations check the encrypt flag on the context. #172

Conversation

Projects
None yet
6 participants
@alokmenghrajani
Copy link
Contributor

commented Sep 8, 2014

I'm new to this codebase, so sorry if I made any style mistakes. Also, I have a couple questions:

  1. do we need to set EVPerr before returning 0;
  2. should we unittest this case?

I believe EVP should make it hard to shoot yourself in the foot, so this change ensures that a user cannot accidentally decrypt data with an encryption context or vice-versa. For example, without the check, if an encryption context is used to decrypt EVP_aes_256_gcm encrypted data, the code will fail to validate the TAG.

Example code availabe at: http://quaxio.com/wtf/openssl_wtf.html

Ensures that EVP encryption & decryption operations check the encrypt…
… flag on the context.

This change ensures that a user cannot accidentally decrypt data with an encryption context
or vice-versa. Without the check, if an encryption context is used to decrypt
EVP_aes_256_gcm encrypted data, the code will fail to validate the TAG.

Example code availabe at: http://quaxio.com/wtf/openssl_wtf.html

@alokmenghrajani alokmenghrajani force-pushed the alokmenghrajani:alok/evp_check_ctx_encrypt branch from 395270c to 4181057 Nov 14, 2014

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2016

I like the concept, but I think returning 0 without setting an error code isn't a good idea.

@alokmenghrajani

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

Agree. Feel free to modify the PR and merge it.

@@ -295,7 +295,7 @@ int EVP_DecryptInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher, ENGINE *im
return EVP_CipherInit_ex(ctx, cipher, impl, key, iv, 0);
}

int EVP_EncryptUpdate(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl,
int EVP_EncryptOrDecryptUpdate(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl,

This comment has been minimized.

Copy link
@levitte

levitte May 3, 2016

Member

This should be made static and with lowercase prefix (i.e. evp_ rather than EVP_)

@levitte

This comment has been minimized.

Copy link
Member

commented May 3, 2016

It's unfortunate that this hasn't been looked at for so long. The code looks correct (apart from the nit I picked), but it needs a rebase to a fresher master.

@levitte levitte self-assigned this May 3, 2016

@levitte levitte added the reviewed label May 6, 2016

@mattcaswell mattcaswell added this to the 1.1.0 milestone May 16, 2016

@mattcaswell

This comment has been minimized.

Copy link
Member

commented May 18, 2016

This PR is from pre-reformat times. Either we should bring it up to date and fix the nit, or abandon it. @levitte?

@levitte

This comment has been minimized.

Copy link
Member

commented May 18, 2016

Like I said, "it needs a rebase to a fresher master"

@mattcaswell

This comment has been minimized.

Copy link
Member

commented May 18, 2016

Ok. So I suggest unless @alokmenghrajani feels like bringing this up to date, we should close this PR.

@levitte

This comment has been minimized.

Copy link
Member

commented May 18, 2016

Agreed

@alokmenghrajani

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2016

Not psyched to go over this ~2 years later.

@alokmenghrajani alokmenghrajani deleted the alokmenghrajani:alok/evp_check_ctx_encrypt branch May 18, 2016

@owlstead

This comment has been minimized.

Copy link

commented Dec 9, 2018

Please don't close issues if they still persist.

See for instance the question on SO in the next URL to see that this causes issues.

https://stackoverflow.com/questions/53690728/openssl-evp-decryption-fails-for-ecb-and-cbc-but-works-for-ofb/53691697#comment94239271_53691697

The routines are currently ignoring the fail fast and least surprise design principles. Please fix.

@kroeckx

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

Feel free to create an other pull request to add those checks.

@levitte

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

Oh fer crying out loud. We goofed by leaving this to rot for years... #7852

@alokmenghrajani

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

@owlstead re "Please don't close issues if they still persist.", this wasn't an issue but a PR. By my books, it's fine to close PRs if no progress is being made.

@levitte Appreciate that you eventually got to fix this issue.

@owlstead

This comment has been minimized.

Copy link

commented Dec 12, 2018

@alokmenghrajani Thank you for initially proposing the code. But your PR consists of code to fix an issue, correct? It doesn't offer any other functionality but to correctly report an error if the API is misused. Otherwise you would have posted "thank you for eventually applying the PR ;). Anyway, fixed is fixed.

@alokmenghrajani

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@owlstead thank you for eventually applying the PR and not keeping the author from the original commit. Sure. Happy?

@owlstead

This comment has been minimized.

Copy link

commented Dec 12, 2018

Hmm, not about the author part, but you're clearly the initiator and it is fixed so yeah - very happy :)

@levitte

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Hmmm, it's true that I didn't refer back to you, @alokmenghrajani, in my remake of this PR. I was a bit exasperated and actually started from scratch, 'cause I thought enough must have changed in master that this PR couldn't be picked cleanly. So yeah, not mentioning you wasn't very courteous of me, and for that I'm sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.