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.

1 Like