Update SymmetricKey.php #1173
Update SymmetricKey.php #1173
Conversation
fix wrong padding string on padded string
|
You're gonna need to provide a little bit more of an explanation for me to merge this in. I'm also not going to merge a PR with failing unit tests... |
| @@ -2629,4 +2638,8 @@ protected function createInlineCryptFunction($cipher_code) | |||
|
|
|||
| return \Closure::bind($func, $this, static::class); | |||
| } | |||
|
|
|||
| public function setCustomPaddingString($str){ | |||
There was a problem hiding this comment.
This method should have DocBlock comment
| } | ||
|
|
||
| $pad = $this->block_size - ($length % $this->block_size); | ||
|
|
||
| if ($this->custom_padding_str != null){ |
There was a problem hiding this comment.
Nit picky but there should be a space in the middle of ){. For otherwise good PR's I'd fix issues like this myself but this PR has other issues that need resolution so while you're in there fixing those other issues might as well fix this issue as well!
|
|
||
| if ($length % $this->block_size == 0) { | ||
| return $text; | ||
| } |
There was a problem hiding this comment.
phpseclib uses PKCS7 padding, when padding is enabled. What that means is that when you have a string whose length is a multiple of the block length you add chr(x) to the end of that string x times, where x is the block length. More info:
https://tools.ietf.org/html/rfc5652#section-6.3
https://en.wikipedia.org/wiki/Padding_(cryptography)#PKCS7
This commit breaks padding when the block size and the pad amount are the same.
This impacts, among other things, encrypted PKCS8 keys.
| } | ||
|
|
||
| $pad = $this->block_size - ($length % $this->block_size); | ||
|
|
||
| if ($this->custom_padding_str != null){ | ||
| return str_pad($text, $length + $pad, $this->custom_padding_str); |
There was a problem hiding this comment.
So what if you have padding enabled and are using a custom padding string and want to decrypt some text? phpseclib will try to unpad using the PKCS7 technique and it'll very possibly fail depending on what the custom padding string is.
If you want to employ custom padding I'd suggest doing it outside of the the SymmetricCipher instance. SSH2.php does this. It has the packet length, the padding length, the data and then random padded data. ie. it doesn't have the SymmetricCipher instance do the padding - it does it itself. Because the padding requires a plaintext that is formatted in a certain way - a format that isn't shared by other protocols or formats or whatever.
fix wrong padding string on padded string