User API keys should use OAEP padding

Feature
1

A minor issue and maybe a bigger one:

There’s a required nonce param that’s not mentioned in the documentation:

  def require_params
    %i[public_key nonce scopes client_id application_name].each { |p| params.require(p) }
  end

Now the trickier issue. Discourse calls the public_encrypt method with no arguments:

That means the padding argument defaults to PKCS1_PADDING. From the Ruby documentation:

Encrypt string with the public key. padding defaults to PKCS1_PADDING, which is known to be insecure but is kept for backwards compatibility.

Unfortunately, Node v20.14.0 (the current LTS) returns an error if you attempt to call crypto.privateDecrypt with RSA_PKCS1_PADDING :

function decryptData(data: string, privateKey: string) {
  const buffer = Buffer.from(data, "base64");
  const decrypted = crypto.privateDecrypt(
    {
      key: privateKey,
      padding: crypto.constants.RSA_PKCS1_PADDING,
    },
    buffer
  );
  return decrypted.toString("utf8");
}

TypeError: RSA_PKCS1_PADDING is no longer supported for private decryption, this can be reverted with --security-revert=CVE-2023-46809

A possible fix for Node apps is to run Node with the insecure flag:

node --security-revert=CVE-2023-46809

A fix on the Discourse end would be easy, but I suspect it would break a lot of existing integrations:

public_key = OpenSSL::PKey::RSA.new(params[:public_key])
@payload = Base64.encode64(public_key.public_encrypt(@payload, OpenSSL::PKey::RSA::PKCS1_OAEP_PADDING))
3 Likes
2

@simon Yes this is definitely causing problems with Node v22. It would be great to not revert security patches. It would be nice to set a flag in the API call or site setting in Discourse to choose the desired padding. (That way people can keep the existing default if they would like.)

1 Like
3

Roughly following the steps here using NodeRSA works

4

This seems like a pretty simple addition?

5

I get that OAEP is recommended for new apps being CCA / Bleichenbach attack resistant. Node forcing our hand here is a bit sad, but I guess this is a “greater good” kind of thing.

I am extremely concerned about making this yet another toggle for a Discourse admin to reason about, that is a nightmare.

Instead we would need to fix Discourse Hub to support the new and old flavors concurrently, have something about our API signal the “version” of the public key.

It is a complicated change that runs through quite a few systems. The fix you proposed is a problem cause then Discourse Hub will stop working for admins that flick to that mode.

3 Likes
7

Thanks for the added context.

To be clear, this is something I have issues it when doing local development. But when connecting to our resources deployed to AWS EC2 instances, it isn’t an issue. I’m guessing their version of Node has some behind-the-scenes customizations or versioning where the crypto library doesn’t have this problem.

8

Coming in cold here but that error seems incorrect. This isn’t a feature that was removed in Node, it’s an issue with some OpenSSL installation. From the Node docs:

Using crypto.constants.RSA_PKCS1_PADDING in crypto.privateDecrypt() requires OpenSSL to support implicit rejection (rsa_pkcs1_implicit_rejection).

See also [Bug]: RSA_PKCS1_PADDING is no longer supported for private decryption · Issue #487 · bropat/eufy-security-client · GitHub

Testing locally, this works for me: An example of RSA Encryption implemented in Node.js · GitHub even when I switch to using crypto.constants.RSA_PKCS1_PADDING for the padding for both encryption and decryption. I am on OpenSSL 3.4.0 and Node 23.6.1.

The tricky thing with using a site setting is that clients won’t know which padding the specific instance is supporting. That makes compatibility across instances/services harder to understand.

I think we should clarify the existing implementation, i.e. explicitly note that we are using RSA_PKCS1_PADDING and then think about an upgrade. Maybe we need to introduce versioning to this endpoint, so that clients can neatly use the right padding before/after said version.

2 Likes
9

For context, this isn’t a feature request by me, it’s just an observation I made in June of last year.

2 Likes