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.

Sign up for GitHub

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

Jump to bottom

Update SymmetricKey.php #1173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update SymmetricKey.php #1173

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 20 additions & 7 deletions phpseclib/Crypt/Common/SymmetricKey.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -2006,17 +2015,17 @@ private function setupMcrypt()
protected function pad($text)
{
$length = strlen($text);

if ($length % $this->block_size == 0) {
return $text;
}
Copy link
Member

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.

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){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

return str_pad($text, $length + $pad, $this->custom_padding_str);
Copy link
Member

Choose a reason for hiding this comment

The 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));
}

Expand Down Expand Up @@ -2629,4 +2638,8 @@ protected function createInlineCryptFunction($cipher_code)

return \Closure::bind($func, $this, static::class);
}

public function setCustomPaddingString($str){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should have DocBlock comment

$this->custom_padding_str = $str;
}
}