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

Use of deprecated libolm for public-key encryption of POST bodies #64

Closed
reivilibre opened this issue Aug 14, 2024 · 12 comments
Closed

Comments

@reivilibre
Copy link
Contributor

reivilibre commented Aug 14, 2024

libolm has been deprecated and the content scanner uses it through python-olm.

We're not aware of any active security problem with using it, but it seems wise to think about this.

The content scanner only uses it for decrypting the bodies of POST requests, using a public-key encryption scheme, in order to prevent snooping by the server performing TLS termination or any node between that one and the content scanner itself.

The code in question is here as below:

def decrypt_body(self, ciphertext: str, mac: str, ephemeral: str) -> JsonDict:
"""Decrypts an Olm-encrypted body.
Args:
ciphertext: The encrypted body's ciphertext.
mac: The encrypted body's MAC.
ephemeral: The encrypted body's ephemeral key.
Returns:
The decrypted body, parsed as JSON.
"""
try:
decrypted = self._decryptor.decrypt(
message=PkMessage(
ephemeral_key=ephemeral,
mac=mac,
ciphertext=ciphertext,
)
)
except PkDecryptionError as e:
logger.error("Failed to decrypt encrypted body: %s", e)
raise ContentScannerRestError(
http_status=400,
reason=ErrCode.FAILED_TO_DECRYPT,
info=str(e),
)
# We know that `decrypted` parses as a JsonDict.
return json.loads(decrypted) # type: ignore[no-any-return]

Vodozemac does not implement the equivalent Olm PkEncryption functionality. (It also sounds like it is unlikely it ever will be implemented as-is, because something is off about the MAC calculation in Olm's current specification — N.B. this MAC issue is no problem for the content scanner)
As far as I know, Element X does not implement support for the content scanner currently. Presumably the lack of implementations of Olm PkEncryption will pose an equivalent problem for them when it's time to implement support.

My suspicion is that this functionality only uses Olm because the clients already had libolm available as a prerequisite for being a E2EE-supporting Matrix client and so it was somewhat convenient to just use what was already available.

Potential solutions:

  1. reimplement this exact operation using the pyca/cryptography library
  2. change the protocol a bit to use some other (more common?) public key cryptography scheme that already has easy-to-use libraries available, e.g. maybe something based on libsodium/NaCl public-key crypto SealedBox. Bear in mind that clients need to use the same scheme here.
    • we could support both in parallel if needed for backwards compatibility
    • this option might make sense because Element X / Vodozemac-using clients will need to reimplement Olm's PkEncryption anyway, so there would be no benefit in sticking with the old over something more standard if it existed
@reivilibre
Copy link
Contributor Author

@MatMaul
Copy link
Contributor

MatMaul commented Aug 28, 2024

If we expose this in the FFI layer of matrix-sdk-crypto it should be fairly trivial to use generated Python bindings for the scanner itself.
I tried to use generated Python bindings for the full SDK for a bot prototype and it seems to work fairly well, including async/await integration (not needed for this use case however).

@MatMaul
Copy link
Contributor

MatMaul commented Aug 29, 2024

I think that's the easiest way forward since it also open the gate to using the FFI layer with new AND old app, and also remove libolm altogether from there.

I have a WIP for adding it to the FFI layer, I am now trying to generate a wheel with maturin, but failing for now.

@poljar
Copy link

poljar commented Sep 3, 2024

If we expose this in the FFI layer of matrix-sdk-crypto it should be fairly trivial to use generated Python bindings for the scanner itself.

But the matrix-sdk-crypto crate does not expose the PkEncryption stuff either.

Perhaps matrix-org/vodozemac#171 and https://github.com/matrix-nio/vodozemac-python would be the better path forward.

@MatMaul
Copy link
Contributor

MatMaul commented Sep 3, 2024

Oh great I didn't know there were some work to bring PkEncryption to vodozemac, it's fine for me too. I went this way because an impl already exists in matrix-sdk-crypto.

@MatMaul
Copy link
Contributor

MatMaul commented Sep 3, 2024

@poljar are the Python bindings supposed to be working ?

After doing a maturin build, I get a wheel with the .so inside, but no generated Python code that would call into it. I am not sure if I am doing something wrong or if it is not in a working state yet.

@poljar
Copy link

poljar commented Sep 3, 2024

Well there are Python tests that are run as part of the CI, so they should™ work.

@MatMaul
Copy link
Contributor

MatMaul commented Sep 3, 2024

So it turns out that a .so can be a self describing Python module, and doesn't need a matching .py file on the side or anything. Learning new stuff every day :)

Thanks, it indeed works.

@MatMaul
Copy link
Contributor

MatMaul commented Sep 4, 2024

I just pushed matrix-nio/vodozemac-python#4.

@devonh
Copy link
Member

devonh commented Sep 18, 2024

I have PR #65 up which entirely removes libolm by using the rust sdks directly.
This approach avoids the extra burden of having to maintain the python bindings for those sdks.
It keeps everything nicely contained here with very simple python wrappers for the rust code.

@devonh
Copy link
Member

devonh commented Sep 19, 2024

libolm is no longer a dependency as of v1.1.0 🎉

@devonh devonh closed this as completed Sep 19, 2024
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

4 participants