web-push-libs / web-push-php

Web Push library for PHP
MIT License
1.71k stars 298 forks source link

Encryption::createLocalKeyObjectUsingPurePhpMethod() creates invalid key #301

Closed rfool closed 3 years ago

rfool commented 4 years ago

Please confirm the following:

Setup

Please provide the following details, the more info you can provide the better.

Please check that you have installed and enabled these PHP extensions :

Problem

When Encryption::createLocalKeyObjectUsingPurePhpMethod() and Utils::serializePublicKeyFromJWK() is used to create $localPublicKey then this will result in a key (with binary size) slightly larger than the expected 65 bytes.

Thus, Encryption::createContext() will throw with "Invalid server public key length".

Expected

$localPublicKey should have binary size of 65 bytes exactly.

Features Used

Example / Reproduce Case

Should be reproducable with official example, when openssl is not correctly configured.

Other

Of course, the best fix is to configure openssl correctly. However, Encryption::createLocalKeyObjectUsingPurePhpMethod() exists and thus should provide correct results.

Unfortunately, I am not an expert on elliptical curves, GMP, JWT and so on. But I have a guess: the error could be in the calls to BigInteger::toBytes(). By default it prepends a signed bit and represents the number in two's-complement. I think this sign-bit is responsible for enlarging key length.

Current:

new JWK([
    'kty' => 'EC',
    'crv' => 'P-256',
    'x' => Base64Url::encode(self::addNullPadding($publicKey->getPoint()->getX()->toBytes())),
    'y' => Base64Url::encode(self::addNullPadding($publicKey->getPoint()->getY()->toBytes())),
    'd' => Base64Url::encode(self::addNullPadding($privateKey->getSecret()->toBytes())),
])

Suggested fix:

new JWK([
    'kty' => 'EC',
    'crv' => 'P-256',
    'x' => Base64Url::encode(self::addNullPadding($publicKey->getPoint()->getX()->toBytes(false))),
    'y' => Base64Url::encode(self::addNullPadding($publicKey->getPoint()->getY()->toBytes(false))),
    'd' => Base64Url::encode(self::addNullPadding($privateKey->getSecret()->toBytes(false))),
])

With this change naiively applied, the key lengths will be exactly 65 bytes, as expected.

However, I don't know if this "fix" is technically correct. As x and y are coordinates, maybe just d should be handled as unsigned?

martin-ro commented 4 years ago

I get that same error "Invalid server public key length". But seemingly random.

martin-ro commented 4 years ago

I forked the repo and applied the changes suggested by @rfool. I don't get the error anymore. But just like op I'm not an expert in this stuff and what implications these changes might have. Maybe someone with deeper knowledge could chime in.

smilingcheater commented 4 years ago

Thank you @rfool, your suggested fix helped alot.

Spomky commented 4 years ago

Hi,

It looks like you are right. I will create a PR to solve that. In case you are interesting, there is a PR where I propose to completely rebuild this library. I created an example that uses this new branch if you want to test it.

Minishlink commented 3 years ago

Available in 6.0.3, thanks people!