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

Encryption::createLocalKeyObjectUsingPurePhpMethod() creates invalid key #301

Closed
8 tasks done
rfool opened this issue Sep 30, 2020 · 5 comments
Closed
8 tasks done

Comments

@rfool
Copy link

rfool commented Sep 30, 2020

Please confirm the following:

  • I have read the README entirely
  • I have verified in the issues that my problem hasn't already been resolved

Setup

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

  • Operating System: Windows
  • PHP Version: 7.4.8
  • web-push-php Version: 6.0.2

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

  • gmp
  • mbstring
  • curl
  • openssl -

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

  • VAPID Support
  • Sending with Payload

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
Copy link

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

@martin-ro
Copy link

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
Copy link

Thank you @rfool, your suggested fix helped alot.

@Spomky
Copy link
Contributor

Spomky commented Nov 4, 2020

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

Minishlink commented Nov 6, 2020

Available in 6.0.3, thanks people!

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

No branches or pull requests

5 participants