# openssl_(en|de)crypt missing IV

- Thread: https://externals.io/message/48304
- Original archive: https://news-web.php.net/php.internals/48304
- Started by Sara Golemon on 2010-05-18T01:53:18+00:00
- Messages: 11
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #48304 — Sara Golemon — 2010-05-18T01:53:18+00:00

URL: https://externals.io/message/48304#48304

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)

---

## #48305 — Davey Shafik — 2010-05-18T02:19:09+00:00

URL: https://externals.io/message/48304#48305 · In reply to: #48304

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()` (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)

---

## #48306 — Tjerk Anne Meesters — 2010-05-18T02:47:56+00:00

URL: https://externals.io/message/48304#48306 · In reply to: #48305

> 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
>
>
> 
>
> > […]
>
>
> --
> 
> 
>
>



-- 
--
Tjerk

---

## #48307 — Pierre Joye — 2010-05-18T06:41:52+00:00

URL: https://externals.io/message/48304#48307 · In reply to: #48304

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 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".

> 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

---

## #48313 — Rob Richards — 2010-05-18T10:56:18+00:00

URL: https://externals.io/message/48304#48313 · In reply to: #48304

Sara Golemon wrote:
> 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)
>

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

---

## #48314 — Pierre Joye — 2010-05-18T11:02:51+00:00

URL: https://externals.io/message/48304#48314 · In reply to: #48313

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

---

## #48315 — Hannes Magnusson — 2010-05-18T11:15:21+00:00

URL: https://externals.io/message/48304#48315 · In reply to: #48314

> hi,
>
> 
>
> > […]
>
> 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

---

## #48325 — Sara Golemon — 2010-05-18T21:17:51+00:00

URL: https://externals.io/message/48304#48325 · In reply to: #48307

> > […]
> 
> > […]
> 
> 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:

1) Add $iv as a fifth, optional parameter to openssl_(en|de)crypt()
2) Throw a warning if `openssl_encrypt()` is used without an IV
3) 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

---

## #48327 — Pierre Joye — 2010-05-18T21:26:58+00:00

URL: https://externals.io/message/48304#48327 · In reply to: #48325

> > […]
>
> 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:
>
> 1) Add $iv as a fifth, optional parameter to openssl_(en|de)crypt()
> 2) Throw a warning if `openssl_encrypt()` is used without an IV
> 3) 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.

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

---

## #48329 — Sara Golemon — 2010-05-18T21:37:11+00:00

URL: https://externals.io/message/48304#48329 · In reply to: #48327

> > […]
> 
> 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.

---

## #48330 — Pierre Joye — 2010-05-18T21:44:39+00:00

URL: https://externals.io/message/48304#48330 · In reply to: #48329

> > […]
> 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:

> > […]

So yes, putting it at fifth optional argument can work as it won't
change the behavior either.

> > […]
> 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