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

[7.x] Update the encryption algorithm to provide deterministic encryption sizes #31721

Conversation

patrickcarlohickman
Copy link
Contributor

@patrickcarlohickman patrickcarlohickman commented Mar 3, 2020

Resubmission of #31698, now targeted to 7.x since it was released.

What's the issue?

This PR addresses issue #31512. When Laravel encrypts a value, the size of the encrypted result is not deterministic, even though a deterministic block cipher is being used.

Even though it may not be considered a bug (see #31512), this is a very simple fix that may solve an issue for some developers, and prevent unexpected errors for others.

Backwards Compatible?

This PR is fully backwards compatible. It does not change any method signatures and it does not affect decrypting any pre-existing encrypted data.

Will this break anything else?

The JSON spec does not require forward slashes to be escaped, but PHP does it by default. This PR just turns that off in this one instance.

I didn't find anything official, but the StackOverflow consensus about why PHP does this seems to be related to default protection for embedding JSON data <script> tags.

Since this JSON value is not output anywhere or shared with anything else, this option is safe to add here.


Extra information that no one needs...


Who cares?

For the most part, this isn't really an issue. However, if one intends to store the encrypted data in a database, one needs to know the size of the field to create. If a developer isn't careful, they may accidentally chose a value that is too small, causing errors later on.

$results = [];
for ($i = 0; $i < 10000; $i++) {
    $results[] = strlen(encrypt('000000000'));
}
dump(array_count_values($results));

[
    216 => 7425,
    220 => 2548,
    224 => 26,
    228 => 1,
]

Based on the above results, it is feasible that a developer could run a couple tests, always get 216 as a result, and decide it is safe to make the database field a char(216), which will eventually blow up.

Long-winded details...

This issue is caused by forward slashes in base64 encoded values that are json encoded.

When Laravel encrypts a value, it generates an array containing the IV, the encrypted value, and the MAC. It json_encodes this array and returns the base64_encoded value of that.

The IV is randomized, but it is always the same size.

Since the IV is always randomized, the encrypted value is always a different result, but it is always the same size due a deterministic block cipher being used.

The MAC is always the same size.

However, the IV and the encrypted value are both base64 encoded, which includes the forward slash in its alphabet. By default, when json encoding data, PHP will escape forward slashes with backslashes, adding more characters to the json encoded result. These extra escape characters increases the size of the final base64 encoded result.

So, the size of the final encoded result depends on how many forward slashes had to be escaped in the base64 encoded IV and encrypted result.

This PR prevents PHP from escaping the forward slashes, so the resulting size will always be the same.

Let me know if there are any problems or issues.

Thanks,
Patrick

@GrahamCampbell GrahamCampbell changed the title [7.x] Update the encryption algorithm to provide deterministic encryption sizes. [7.x] Update the encryption algorithm to provide deterministic encryption sizes Mar 4, 2020
@GrahamCampbell
Copy link
Member

Maybe best fir this to go to Laravel 8?

@AbdelElrafa
Copy link
Contributor

@GrahamCampbell I don't see any breaking change. This is a bug fix that will normalize a behaviour.

@patrickcarlohickman
Copy link
Contributor Author

patrickcarlohickman commented Mar 4, 2020

Obviously up to you guys, but this really shouldn't break anything for anyone.

Everything I could think of:

  • No method signature changes.
  • The JSON_UNESCAPED_SLASHES option was added in PHP 5.4.0, so it is available in ^7.2.5.
  • Escaping forward slashes is not required by the JSON spec.
  • json_decode() will decode unescaped forward slashes without issue.
  • Existing encrypted data will still decrypt properly.
  • The resulting encrypted size is the smallest possible size, so the result won't be bigger than anyone already accounted for (in the example in the original post above, encrypting the 9 digit string will always return the smallest size of 216 characters).
  • The < and > characters are not part of the base64 alphabet, so in the small possibility someone is base64 decoding the encryption result and outputting the raw json into a <script> tag, there still won't be the possibility of the json value having a </script> tag in it (original reason PHP escapes the forward slashes).
  • Bonus: All the encrypter tests still pass.

Here is an example json result that gets base64 encoded and returned as the result from the encrypt function.

{
    "iv": "gWHjzJRJbcC3jk\/rM85HOA==",
    "value": "rN\/\/tKWFM7S+6O+5MPmEhQ==",
    "mac": "c950df930356c14be9a668ff0945e19c00ae91295dae0b01c7b4c51d46c37777"
}

The only thing this PR does is prevent escaping the forward slashes (found in the iv and value):

{
    "iv": "gWHjzJRJbcC3jk/rM85HOA==",
    "value": "rN//tKWFM7S+6O+5MPmEhQ==",
    "mac": "c950df930356c14be9a668ff0945e19c00ae91295dae0b01c7b4c51d46c37777"
}

This means that the json result will be a consistent size, since it won't fluctuate based on how many forward slashes were escaped. Additionally, the consistent size is the smallest possible size, so it won't break anything for anyone that has already allocated space for storing encrypted data.

If there are any questions or issues, please let me know.

Thanks,
Patrick

@taylorotwell taylorotwell merged commit 19c9830 into laravel:7.x Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants