-
-
Notifications
You must be signed in to change notification settings - Fork 877
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
Update SymmetricKey.php #1173
base: master
Are you sure you want to change the base?
Update SymmetricKey.php #1173
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -466,6 +466,15 @@ abstract class SymmetricKey | |
*/ | ||
protected $explicit_key_length = false; | ||
|
||
|
||
/** | ||
* use for add predefine padding string? | ||
* | ||
* @see self::setCustomPaddingString() | ||
* @var string | ||
* @access protected | ||
*/ | ||
protected $custom_padding_str = null; | ||
/** | ||
* Default Constructor. | ||
* | ||
|
@@ -2006,17 +2015,17 @@ private function setupMcrypt() | |
protected function pad($text) | ||
{ | ||
$length = strlen($text); | ||
|
||
if ($length % $this->block_size == 0) { | ||
return $text; | ||
} | ||
if (!$this->padding) { | ||
if ($length % $this->block_size == 0) { | ||
return $text; | ||
} else { | ||
throw new \LengthException("The plaintext's length ($length) is not a multiple of the block size ({$this->block_size}). Try enabling padding."); | ||
} | ||
throw new \LengthException("The plaintext's length ($length) is not a multiple of the block size ({$this->block_size}). Try enabling padding."); | ||
} | ||
|
||
$pad = $this->block_size - ($length % $this->block_size); | ||
|
||
if ($this->custom_padding_str != null){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit picky but there should be a space in the middle of |
||
return str_pad($text, $length + $pad, $this->custom_padding_str); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
return str_pad($text, $length + $pad, chr($pad)); | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method should have DocBlock comment |
||
$this->custom_padding_str = $str; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.