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

Added the ability to sign and verify closure serializations. #43

Merged
merged 1 commit into from
Mar 11, 2015

Conversation

jeremeamia
Copy link
Owner

No description provided.

@@ -22,6 +24,7 @@ public function serialize(\Closure $closure);
* @param string $serialized Serialized closure.
*
* @return \Closure Unserialized closure.
* @throws ClosureUnserializationException if unserialization fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect order of annotations here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm unaware of any ordering rules. Link?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something' that's implied by PSR-5 in their code examples. I'm also developing docblock fixers for php-cs-fixer that enforce the order param, throws, return for the annotations.

PHP-CS-Fixer/PHP-CS-Fixer#887

Copy link
Contributor

Choose a reason for hiding this comment

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

NB, I did actually run regression testing on the aws sdk at one point. :)

@GrahamCampbell
Copy link
Contributor

I suspect a lot of people will use the serializer class to serialize, but end up using php's unserialize function later in their code, kind of voiding this. Laravel does this currently (or rather, the equivalent in the 1.x version).

@jeremeamia
Copy link
Owner Author

I attempted a solution that would work that way, but was met with segfault after segfault.

@GrahamCampbell
Copy link
Contributor

:(

@GrahamCampbell
Copy link
Contributor

I don't think adding this into 2.0 is a good idea. I think we should probably wait for feedback on it and put it into 2.1.

@ircmaxell
Copy link

Just for form, can the unrelated readme changes be removed from the PR?

@@ -61,9 +79,20 @@ public function serialize(\Closure $closure)
*/
public function unserialize($serialized)
{
/** @var SerializableClosure $unserialized */
$unserialized = unserialize($serialized);

Choose a reason for hiding this comment

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

The signature should be appended or prepended to the result. It shouldn't be "re-serailized" into a new one. That's because unserializing user input can be disruptive. See http://blog.codeclimate.com/blog/2013/01/10/rails-remote-code-execution-vulnerability-explained/

Instead, consider prepending the signature, either base64 encoded or hex encoded. That way you can just substr it off .

So unserializing user input is a no-no. Instead, something like this should be done:

if ($this->signingKey) {
    if ($signature[0] !== '%') {
        throw new \ClosureUnserializationException('Attempting to verify unsigned serialization with a key');
    }
    $signature = substr($serialized, 1, 88);
    $data = substr($serialized, 89);
    if (!hash_equals(base64_decode($signature) , hash_hmac('sha256', $data, $this->signingKey))) {
        throw new ...
    }
    $unserialized = unserialize($data);
} elseif ($signature[0] == '%') {
    throw new \ClosureUnserializationException('Attempting to unserialize signed serialization without a key');
}

The % check is just a quick way to force verification, and since it's invalid as a first character in a serialization, it shouldn't be a problem...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Cool, that makes sense.

@Ocramius
Copy link

Very nice feature. You should also factor the lib version in the hash tho.

@jeremeamia
Copy link
Owner Author

@ircmaxell I updated the pull request incorporating your recommendations. Please take another look and make sure I'm on track. Thanks.


$this->verifySignature($signature, $serialized);
}

/** @var SerializableClosure $unserialized */
$unserialized = unserialize($serialized);

Choose a reason for hiding this comment

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

You may want to check for % here if signing key is not set (and hence strip off the token). As if you don't verify a signed serialization will not be unserializable otherwise.

@jeremeamia jeremeamia changed the title Added closure serialization signing and updated the README. Added the ability to sign and verify closure serializations. Mar 11, 2015
jeremeamia added a commit that referenced this pull request Mar 11, 2015
Added the ability to sign and verify closure serializations.
@jeremeamia jeremeamia merged commit cb1b417 into master Mar 11, 2015
@GrahamCampbell GrahamCampbell deleted the signing branch March 23, 2015 15:50
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