I was just looking through the implementation of openssl_encrypt()
(and
openssl_decrypt()
) today because I need to make some encrypted payloads,
but the prototype didn't have anywhere to place an initialization vector.
On opening ext/openssl/openssl.c, I noticed line 4620 which simply
hardcodes IV as a string of NULL
bytes.
This is a bad idea roughly equivalent to hashing passwords without salt;
Worse, it prevents interoperability at the application layer by
preventing the decryption of a data stream where the generator used an
IV other than all-null.
Fixing this is a simple matter, but I wanted to bounce approaches for BC
(or lack thereof) off everyone else since this version of
openssl_encrypt()
is already "in the wild".
The most direct and obvious solution is to add a fifth, optional
parameter to openssl_encrypt()
and openssl_decrypt()
to take IV as a
string. The problems with this are that it: (A) Places the IV in an odd
argument location, and (B) Does not enforce the passing of an IV (since
raw is already optional). As stated above, IV really should be
enforced, given what it does to soften the security normally offered by
a chaining block method.
That said, I'd like to propose something unpopular; Change the
signature for these methods entirely, deliberately breaking BC. I know,
I know.... spare me your rotten tomatoes. I think it's justified in
this case because, as they are now, these functions are useless at best,
and possibly dangerous in terms of encouraging unsafe practices with
regards to cryptography.
I think it's worth a BC break. Comments?
-Sara
P.S. - Here's the signature I'd go with: openssl_encrypt($data, $method,
$iv, $key, $raw=false)
The least disruptive change would be to have it as the last arg, and default to the current all-null value.
Perhaps you could do this and add a warning akin to the date.timezone if none is passed?
Having said that, I don't think the disruption would be too bad, I haven't seen much use of the openssl stuff in the
wild; it's pretty much pointless anyways, everybody knows you can decrypt anything with a modem coupler and
any 15 year old kid...
Another option, is an openssl_set_iv($iv); method, that would setup for openssl_encrypt/decrypt to use, otherwise
use the current all-null option....
This has the added benefit of being as global as you like, and no need to keep passing the IV to encrypt/decrypt
all over the place. It has the potential of clashing when you bring multiple codebases together however (i.e a framework).
Possible to limit to a single namespace?
- Davey
I was just looking through the implementation of
openssl_encrypt()
(andopenssl_decrypt()
) today because I need to make some encrypted payloads, but the prototype didn't have anywhere to place an initialization vector.On opening ext/openssl/openssl.c, I noticed line 4620 which simply hardcodes IV as a string of
NULL
bytes.This is a bad idea roughly equivalent to hashing passwords without salt; Worse, it prevents interoperability at the application layer by preventing the decryption of a data stream where the generator used an IV other than all-null.
Fixing this is a simple matter, but I wanted to bounce approaches for BC (or lack thereof) off everyone else since this version of
openssl_encrypt()
is already "in the wild".The most direct and obvious solution is to add a fifth, optional parameter to
openssl_encrypt()
andopenssl_decrypt()
to take IV as a string. The problems with this are that it: (A) Places the IV in an odd argument location, and (B) Does not enforce the passing of an IV (since raw is already optional). As stated above, IV really should be enforced, given what it does to soften the security normally offered by a chaining block method.That said, I'd like to propose something unpopular; Change the signature for these methods entirely, deliberately breaking BC. I know, I know.... spare me your rotten tomatoes. I think it's justified in this case because, as they are now, these functions are useless at best, and possibly dangerous in terms of encouraging unsafe practices with regards to cryptography.
I think it's worth a BC break. Comments?
-Sara
P.S. - Here's the signature I'd go with: openssl_encrypt($data, $method, $iv, $key, $raw=false)
The least disruptive change would be to have it as the last arg, and default to the current all-null value.
Perhaps you could do this and add a warning akin to the date.timezone if none is passed?
Having said that, I don't think the disruption would be too bad, I haven't seen much use of the openssl stuff in the
wild; it's pretty much pointless anyways, everybody knows you can decrypt anything with a modem coupler and
any 15 year old kid...
Don't think this warrants a notice/warning in existing code; for those
who are concerned about this, a simple release note stating a fifth
parameter has been added to openssl_encrypt should suffice IMO.
That said, having a "truly" random and big enough IV (OTP) should keep
hackers away for some time.
There are also alternatives that already accept IV in their encrypt()
methods; mcrypt comes to mind ;-)
Another option, is an openssl_set_iv($iv); method, that would setup for openssl_encrypt/decrypt to use, otherwise
use the current all-null option....This has the added benefit of being as global as you like, and no need to keep passing the IV to encrypt/decrypt
all over the place. It has the potential of clashing when you bring multiple codebases together however (i.e a framework).
Possible to limit to a single namespace?
- Davey
I was just looking through the implementation of
openssl_encrypt()
(andopenssl_decrypt()
) today because I need to make some encrypted payloads, but the prototype didn't have anywhere to place an initialization vector.On opening ext/openssl/openssl.c, I noticed line 4620 which simply hardcodes IV as a string of
NULL
bytes.This is a bad idea roughly equivalent to hashing passwords without salt; Worse, it prevents interoperability at the application layer by preventing the decryption of a data stream where the generator used an IV other than all-null.
Fixing this is a simple matter, but I wanted to bounce approaches for BC (or lack thereof) off everyone else since this version of
openssl_encrypt()
is already "in the wild".The most direct and obvious solution is to add a fifth, optional parameter to
openssl_encrypt()
andopenssl_decrypt()
to take IV as a string. The problems with this are that it: (A) Places the IV in an odd argument location, and (B) Does not enforce the passing of an IV (since raw is already optional). As stated above, IV really should be enforced, given what it does to soften the security normally offered by a chaining block method.That said, I'd like to propose something unpopular; Change the signature for these methods entirely, deliberately breaking BC. I know, I know.... spare me your rotten tomatoes. I think it's justified in this case because, as they are now, these functions are useless at best, and possibly dangerous in terms of encouraging unsafe practices with regards to cryptography.
I think it's worth a BC break. Comments?
-Sara
P.S. - Here's the signature I'd go with: openssl_encrypt($data, $method, $iv, $key, $raw=false)
--
--
--
Tjerk
hi Sara,
I was just looking through the implementation of
openssl_encrypt()
(and
openssl_decrypt()
) today because I need to make some encrypted payloads, but
the prototype didn't have anywhere to place an initialization vector.On opening ext/openssl/openssl.c, I noticed line 4620 which simply hardcodes
IV as a string ofNULL
bytes.This is a bad idea roughly equivalent to hashing passwords without salt;
Worse, it prevents interoperability at the application layer by preventing
the decryption of a data stream where the generator used an IV other than
all-null.Fixing this is a simple matter, but I wanted to bounce approaches for BC (or
lack thereof) off everyone else since this version ofopenssl_encrypt()
is
already "in the wild".
I think it's worth a BC break. Comments?
To break BC is a no go, even if your arguments are appealing (even in
a major version).
I would suggest a new function:
openssl_encrypt_iv($data, $method, $key, $iv, $raw=false);
Which will use the same internal implementations internally but with a
different entry point. please note that I moved iv to the 4th position
as well.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Fixing this is a simple matter, but I wanted to bounce approaches for BC (or
lack thereof) off everyone else since this version ofopenssl_encrypt()
is
already "in the wild".I think it's worth a BC break. Comments?
To break BC is a no go, even if your arguments are appealing (even in
a major version).
I disagree about it's no-go-ness, given the fact that these functions
aren't particularly usable as-is, but it's also not worth a fight.
Given the comments made on list my intentions are as follows:
- Add $iv as a fifth, optional parameter to openssl_(en|de)crypt()
- Throw a warning if
openssl_encrypt()
is used without an IV - Add openssl_cipher_get_iv_length($cipher)
I intend to make these changes on both trunk and PHP_5_3 because, IMO,
this is a bug, not merely a missing feature.
The only BC break is the warning raised when using openssl_encrypt()
without an IV. Given the extremely bad practice using a NULL
IV
represents, I think this is a reasonable course of action.
-Sara
Fixing this is a simple matter, but I wanted to bounce approaches for BC
(or
lack thereof) off everyone else since this version ofopenssl_encrypt()
is
already "in the wild".I think it's worth a BC break. Comments?
To break BC is a no go, even if your arguments are appealing (even in
a major version).I disagree about it's no-go-ness, given the fact that these functions aren't
particularly usable as-is, but it's also not worth a fight.Given the comments made on list my intentions are as follows:
- Add $iv as a fifth, optional parameter to openssl_(en|de)crypt()
- Throw a warning if
openssl_encrypt()
is used without an IV- Add openssl_cipher_get_iv_length($cipher)
I intend to make these changes on both trunk and PHP_5_3 because, IMO, this
is a bug, not merely a missing feature.The only BC break is the warning raised when using
openssl_encrypt()
without
an IV. Given the extremely bad practice using aNULL
IV represents, I think
this is a reasonable course of action.
It changes the signature making the fifth argument a complete
different thing. I strongly disagree with this strategy, even if I can
understand your reasoning. We can't begin to "fix" things by changing
existing API signatures, that's going to end badly.
To define a new and clean API is not only BC but may also allow
obvious naming. And last but not least, it can then be merged in 5.3
without worrying about any possible impact.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
The only BC break is the warning raised when using
openssl_encrypt()
without
an IV. Given the extremely bad practice using aNULL
IV represents, I think
this is a reasonable course of action.It changes the signature making the fifth argument a complete
different thing. I strongly disagree with this strategy, even if I can
understand your reasoning. We can't begin to "fix" things by changing
existing API signatures, that's going to end badly.
What do you mean by "making the fifth argument a complete different
thing"? Different to what? The current signature only has four
arguments. This adds an optional argument which, by itself, is not BC
breaking.
If you really feel strongly about it, I can add it as a new function
entirely, but the duplication seems very unnecessary to me.
I'm all for preserving BC, but when one does so at the sacrifice of
reason* it just makes us all look a bit silly.
To define a new and clean API is not only BC but may also allow
obvious naming. And last but not least, it can then be merged in 5.3
without worrying about any possible impact.
Absolutely true, and /possibly/ worth the "wtf" that comes with having
two essentially identical functions. How would you feel about the trunk
versions of the old functions (but not the 5.3 versions) gaining
ZEND_ACC_DEPRECATED in that case?
-Sara
- Once again, the current 5.3 implementation is worthless, it is without
worth, it has no value of any worth, it's worthless Dave.
The only BC break is the warning raised when using
openssl_encrypt()
without
an IV. Given the extremely bad practice using aNULL
IV represents, I
think
this is a reasonable course of action.It changes the signature making the fifth argument a complete
different thing. I strongly disagree with this strategy, even if I can
understand your reasoning. We can't begin to "fix" things by changing
existing API signatures, that's going to end badly.What do you mean by "making the fifth argument a complete different thing"?
Different to what? The current signature only has four arguments. This
adds an optional argument which, by itself, is not BC breaking.
I misread this paragraph, I kept in mind what you said in your initial post:
P.S. - Here's the signature I'd go with: openssl_encrypt($data, $method, $iv, $key, $raw=false)
So yes, putting it at fifth optional argument can work as it won't
change the behavior either.
To define a new and clean API is not only BC but may also allow
obvious naming. And last but not least, it can then be merged in 5.3
without worrying about any possible impact.Absolutely true, and /possibly/ worth the "wtf" that comes with having two
essentially identical functions. How would you feel about the trunk
versions of the old functions (but not the 5.3 versions) gaining
ZEND_ACC_DEPRECATED in that case?
Having new cleaner APIs is what I would prefer to do (in general). And
we can indeed add the deprecated flag to 5.3 as well.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Sara Golemon wrote:
I was just looking through the implementation of
openssl_encrypt()
(andopenssl_decrypt()
) today because I need to make some encrypted
payloads, but the prototype didn't have anywhere to place an
initialization vector.On opening ext/openssl/openssl.c, I noticed line 4620 which simply
hardcodes IV as a string ofNULL
bytes.This is a bad idea roughly equivalent to hashing passwords without
salt; Worse, it prevents interoperability at the application layer by
preventing the decryption of a data stream where the generator used an
IV other than all-null.Fixing this is a simple matter, but I wanted to bounce approaches for
BC (or lack thereof) off everyone else since this version of
openssl_encrypt()
is already "in the wild".The most direct and obvious solution is to add a fifth, optional
parameter toopenssl_encrypt()
andopenssl_decrypt()
to take IV as a
string. The problems with this are that it: (A) Places the IV in an
odd argument location, and (B) Does not enforce the passing of an IV
(since raw is already optional). As stated above, IV really should
be enforced, given what it does to soften the security normally
offered by a chaining block method.That said, I'd like to propose something unpopular; Change the
signature for these methods entirely, deliberately breaking BC. I
know, I know.... spare me your rotten tomatoes. I think it's
justified in this case because, as they are now, these functions are
useless at best, and possibly dangerous in terms of encouraging unsafe
practices with regards to cryptography.I think it's worth a BC break. Comments?
-Sara
P.S. - Here's the signature I'd go with: openssl_encrypt($data,
$method, $iv, $key, $raw=false)
Personally I would like to see the signature changed to be even more
aligned with the ones from mcrypt - since thats what people are most
used to using. Currently without the iv, these methods are useless imo.
The biggest downfall from changing the signature where the parameter is
not optional, is that it cant be done until a major release, which will
drag out the time where the functionality will be useful to anyone.
When the iv parameter is added (however it is done), also add
functionality for creating an iv and getting iv length based on method.
These methods are crucial for the function to be useful in real world apps.
Rob
hi,
Personally I would like to see the signature changed to be even more aligned
with the ones from mcrypt - since thats what people are most used to using.
I would like to as well but we can't. To change method signatures in a
way that is not BC compatible is really a bad thing to do.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
hi,
Personally I would like to see the signature changed to be even more aligned
with the ones from mcrypt - since thats what people are most used to using.I would like to as well but we can't. To change method signatures in a
way that is not BC compatible is really a bad thing to do.
ext/openssli ? :)
-Hannes