Voice transport negotiation and AES-256-GCM #4824
Conversation
|
Excellent job! The negotiation concept looks good to me, however I would not rely on a fixed list of supported ciphers. We can simply check whether EVP_get_cipherbyname() returns a valid pointer and then adjust key, iv and tag size through the other OpenSSL functions. |
|
@davidebeatrici Make sense! We can implement an abstract class for the ciphers provided by EVP interface and set the key/iv size in the inherited class. Fixed list of ciphers doesn't look very good as well. Maybe I can initialize a list of ciphers somewhere. |
There was a problem hiding this comment.
A few more remarks:
- I would write constants in all-caps. That makes it obvious that they are constants when used outside of their definition.
- In general I think using
uint32_t(and its friends) are the more modern (better?) way of using unsigned integer types (aka: instead ofunsigned int) - A little more documentation on the new classes could be useful
- Please drop the translation commit, rebase against current master and then use the new python script to update the translations
- Sorry for the myriad of review comments xD
|
@Krzmbrzl Thanks for the thorough review :)
Make sense.
Make sense.
Make sense.
I had an old problem with using the translation script, as
No worries :) I will carefully read them one by one. |
|
In terms of the dynamic cypher list as returned by OpenSSL: From a maintenance point of view I think it'd be greatly advantageous to have a fixed list of supported ciphers instead of having an open list. Note that we'd have to always stay up-to-date about the ciphers provided by OpenSSL (in terms of potential security breaches) in order to offer blacklists (we can't expect that from the average user). Plus there might be a myriad of exotic ciphers that for some reason cause problems, that we'd receive as bug reports. We could however still check for the available OpenSSL ciphers and all ciphers that are not part of our fixed list, we could mark as |
|
Regarding why I gave up adding My initial thought was, first try to negotiate and establish a UDP connection. If the client discovers the UDP pathway is blocked, the client initiates the negotiation again without all UDP options and finally reaches an agreement with the server to use But this is not how Mumble works. The current model is: the voice transport will be sent through UDP first, along with UDP ping message. If Mumble discovered that no ping was received in a certain time interval, it will switch to the TCP mode (search I was having a problem implementing this auto-adaptive measure. According to the negotiation model, after falling back to the TCP connection, I can't send UDP ping messages to the server because ping messages through the UDP connection are encrypted. If I have re-negotiated with the server to use the TCP mode, how can I instruct the server to initialize the cipher, sync with me, and be ready to listen to my UDP message? A solution is to create something called "secondary voice transport". This way, I can specify a UDP transport in the first place and supply the |
I agree 👍 Something else that I came to my mind recently: I did not check the code whether it makes sure that the officially supported protocols have to start with |
🤔️ Wondering which TODO do you mean.
If you mean the protocol name string, I didn't add such prefixes. Do you mean something like |
|
Yes that's probably the one I remembered. It is true though that sending UDP statistics for s TCP connected client doesn't make sense. Yes I do mean that String. Though really only the string that gets sent around in the proto messages. Once it's inside Mumble, we don't need it anymore. |
src/crypto/CryptStateAES256GCM.cpp
Outdated
| void CryptStateAES256GCM::genKey() { | ||
| RAND_bytes(raw_key, keyLength); | ||
| RAND_bytes(encrypt_iv, ivLength); | ||
| RAND_bytes(decrypt_iv, ivLength); | ||
| bInit = true; | ||
| } |
There was a problem hiding this comment.
https://www.openssl.org/docs/man1.1.1/man3/RAND_bytes.html:
If the entropy source fails or is not available, the CSPRNG will enter an error state and refuse to generate random bytes. For that reason, it is important to always check the error return value of RAND_bytes() and RAND_priv_bytes() and not take randomness for granted.
There was a problem hiding this comment.
@Krzmbrzl, @davidebeatrici, the OCB2 implementation does this which can be a security issue:
mumble/src/crypto/CryptStateOCB2.cpp
Lines 44 to 51 in f5177b8
There is this but the implementation is bad:
mumble/src/crypto/CryptographicRandom.cpp
Lines 14 to 31 in f5177b8
Consider using https://doc.libsodium.org/generating_random_data.
There was a problem hiding this comment.
So the OCB2 implementation does have the same flaw of not checking the return value. How is the CryptographicRandom class related to that though? As a potential replacement for the RAND_bytes macro? And why is that latter implementation bad?
There was a problem hiding this comment.
CryptographicRandom::fillBuffer seemingly was added as a way to use RAND_bytes correctly but is not used by anything. Calling qFatal instead of retrying is bad.
Again, libsodium's interface would be better because it handles this automatically, uses the correct parameter types, and uses more secure kernel CSPRNGs rather than OpenSSL's CSPRNG.
There was a problem hiding this comment.
Alright. Thanks for the clarification 👍
There was a problem hiding this comment.
Correction: CryptographicRandom::fillBuffer is indirectly used by PasswordGenerator::generatePassword which indirectly uses https://github.com/mumble-voip/mumble/tree/master/3rdparty/arc4random. Sodium provides randombytes_uniform which could be used to remove the arc4random library.
|
Memo:
Meanwhile, I think it is a little bit too hard if I try to fix my previous commits through rebasing. I will use a new commit to do the changes proposed by @Krzmbrzl in his review. |
So for the time being If we weren't using Strings, I'd propose assembling special numbers whose first 32bit describe the used protocol and the second 32bit describe the cipher.
You'll have to do the rebasing eventually anyways, since we want the commit history to be clean at the point we will merge this PR. If you think that it is easier for you to add new commits now and only squash the commits right before merging that's fine though. Btw your list is missing the prefix for the String representation of officially supported protocols/ciphers ☝️ |
I have an impression that an
I will do my best to squash some commits, but actually, your suggestions scatter across several commits and it is hard for me to put a modification to the right commit that contains it without messing the whole thing up or making my life too hard. I would probably squash all suggestions you have proposed so far into the (current) last commit of this PR, and then start to finish the checklist mentioned above.
Added :) |
Yes that is the case. Can fields in a struct be optional? In that case don't mind my comment above :D
My suggestion would be to fix all review comments in a separate commit and then once everything has been addressed, rebase everything and squash the fixes into the respective base commit (where that area of the code has been introduced. Alternatively rebase and edit your current commits, applying suggestions as applicable for the state of the code at that commit. |
|
In another context it was mentioned that maybe DTLS might be a good fit for Mumble and after having read a paper describing how it works, I think this would be a near perfect fit for us. That is because in contrast to the current approach (and also the approach implemented in this PR) which requires a separate TCP channel over which the cipher negotiation happens, DTLS can do all of that directly in the UDP channel. This includes resyncs should there be any issues during communication. I think this would reduce the complexity of our crypto implementation and by using e.g. OpenSSL we have a library that already takes care of the heavy-lifting and offering us a wide variety of available ciphers. Therefore I currently think that it might be the better solution to implement DTLS instead of implementing essentially the same functionality by ourselves (which apart from being work-intense is then naturally prone to errors being made at some point). It is very unfortunate that I only found out about this now after there has been this much work invested in this PR, so I am sorry for that. Nevertheless I think we should seriously consider DTLS as an alternative. /CC @TerryGeng @davidebeatrici @Avatat @TredwellGit - your opinions on this? |
|
@jedisct1 expressed concerns about it in #3918 (comment). |
|
Ho - guess I have not followed that discussion that attentively xD |
|
Hi! I think I have finally settled things down and have some time now to continue working on this PR. But just to be sure, during the vicinity of my absence, is there any update on this matter? Should I continue or wait for the result of the new discussion of DTLS in #3918? |
The only update on this matter is that I figured out what DTLS is and that it seems to solve a standard way of what we are trying to achieve :)
I would at least wait a bit to see where the discussion leads 🤔 |
|
@TerryGeng I think the point that was made in #3918 (that DTLS should simply be another option within the negotiation framework) was very good. As such I think you can continue with this PR without any further concerns about DTLS as we can add that option at a later point :) |
|
@Krzmbrzl Thanks to let me know! I will continue working on this PR next week. |
…gotiation. See Mumble.proto for more details. Mumble doesn't support other voice protocol so far, so this message is intended to be used to negotiate the cipher type used in transportation. It is sent between Version and Authenticate message, before the CryptState is sent from the server when handling the Authenticate message. A config option "voiceProtocolPreference" is also added to allow the server administrator to specify the protocol allowed by the server. This feature implements the idea of mumble-voip#3918 and mumble-voip#4248.
The encrypted output of block cipher will always be the same as the input length. However, authenticated encryption algorithms produces "tag" to verify the authenticity of the cipher text and this "tag" need to be attached to the cipher text. This tag may vary from cipher to cipher, therefore it is logical to define a custom "head length" for each cipher.
src/crypto/CryptStateEVP.h
Outdated
| raw_key = new unsigned char[KEY_LENGTH]; | ||
| encrypt_iv = new unsigned char[IV_LENGTH]; | ||
| decrypt_iv = new unsigned char[IV_LENGTH]; | ||
| decrypt_history = new unsigned char[0x100]; |
There was a problem hiding this comment.
I explicitly allocate these members here, because a weird bug would happen if I use automatic variables like unsigned char raw_key[KEY_LENGTH] that causes all four arrays here to have the same address. Weird enough, really.
There was a problem hiding this comment.
Alright... This is not some weird bug. This is just because things like KEY_LENGTH was not assigned with a meaningful value when this class is being constructed.
There was a problem hiding this comment.
Note also that variable-size arrays on the stack are not supported in Standard C++. They are supported by GCC (and Clang I think) but last time I checked they were unsupported by MSVC.
That being said though, you should consider using smart-pointers (std::unique_ptr) for these arrays to avoid having to do manual memory management.
There was a problem hiding this comment.
Actually however I the *_LENGTH are constexpr, aren't they? In that case this should work (the arrays on the stack I mean). At least as long as the sizes don't get too big. What order of magnitude are we talking about here for these sizes?
There was a problem hiding this comment.
A few bytes. Not a big deal. The problem arises when different ciphers require different values of *_LENGTH, like the TAG_LENGTH for AES-156-GCM is 8 while for CHACHA it is 16.
I would like to override these values(originally declared in CryptStateEVP) differently from both CryptStateAES256GCM and CryptStateChacha20Poly1305, but override a const value is tricky because of object slicing. That's the reason I use a template for CryptStateEVP.
There was a problem hiding this comment.
The nice thing about my suggestion is that we don't have to allocate anything on the heap. All arrays can be constructed with compile-time constant sizes.
And I think we can have the best of all worlds: For now the only difference is in some constants, so let's encapsulate them in some dummy objects as outlined by me (I also prefer your naming scheme of using EVPCipher::XXXX) and then we can add the respective typedefs for the specific types:
using CryptStateAES256GCM = CryptStateEVP< EVPCipher::AES_156_GCM >;
// ...And once we actually need to override functions, the new cipher can then be an actual class extending CryptStateEVP instead of merely being a typedef. For that to work we'd only have to make sure the respective functions are declared virtual in the base class (unless we will never access these objects via their super-type).
There was a problem hiding this comment.
Yeah. This sounds good. We can still extend CryptStateEVP if needed by inheriting.
Regarding using std::array or c-style array, I think std array certainly has some pros, however, the EVP interface of OpenSSL takes pointer of c-style array to operate and AFAIK there's no way to get the underlying c-array from std::array.
There was a problem hiding this comment.
I'm trying this idea but somehow the compiler complains about static variables being redefined for some test cases, while it does work it I don't include those test cases. I'm confused since the #ifndef should prevent them from being redefined. Do you have any idea about what is wrong?
There was a problem hiding this comment.
I see the problem. The definitions of these variables were compiled into multiple object files and the problems arise when linking. I have moved these definitions into a separate .cpp file.
There was a problem hiding this comment.
AFAIK there's no way to get the underlying c-array from std::array
Yes there is:
https://en.cppreference.com/w/cpp/container/array/data
The definitions of these variables were compiled into multiple object files
Which variables are we talking about? 👀
TerryGeng
force-pushed
the
capabilities
branch
2 times, most recently
from
5fc583c to
7f70075
Compare
|
Alright. After some initial testing, I think this PR is, again, ready for review. I have no plan to add ciphers from OpenSSL other than AES-256-GCM and Chacha20-Poly1305, which have been extensively discussed and tested in #3918. But the I have made several notes in the new commits. I encountered several difficulties when coding (learned a lot of C++ in the middle XD) and I'm not sure my solution is the proper solution. I'm happy to continue working on it if we come up with better ideas. Now 1.4.x is in its final stage before releasing while 1.5.x is still far away, I believe we have plenty of time to go through these. :D |
…pher mechanism, by using more dynamic C++ features: - Created a more complex VoiceProtocol struct to parse the protocol string, - Created CryptStateFactory to handle the creation of CryptState and manage available ciphers.
TerryGeng
force-pushed
the
capabilities
branch
5 times, most recently
from
ab4e9e1 to
234489c
Compare
OpenSSL's EVP provides a unified interface to invoke AEAD ciphers. In this commit, I provide a generalized CryptStateEVP to invoke EVP. This new CryptStateEVP ease the pain of adding new ciphers from EVP.
|
|
||
| bool isValid() const override; | ||
| void genKey() override; | ||
| bool setKey(const std::string &rkey, const std::string &eiv, const std::string &div) override; |
There was a problem hiding this comment.
Isn't GCM supposed not to need an IV, or rather to calculate it from the key in a deterministic manner? Or does "IV" just mean "per-message nonce" here?
There was a problem hiding this comment.
Actually, GCM needs an IV. You can actually find a recommendation for IV generation in the link below:
mumble/src/crypto/EVPCiphers.h
Lines 14 to 24 in d20d3e9
There was a problem hiding this comment.
Oh, so there's a separate API exposed somewhere that just takes the nonce and does the rest for us, like NaCl does?
There was a problem hiding this comment.
Yes. It is combined in the init function.
mumble/src/crypto/CryptStateEVP.h
Lines 138 to 139 in d20d3e9
| // Sent after the version packet, indicate the protocol preference of the client. | ||
| // - This message is not required for text-only clients. It can be sent before or after | ||
| // the Authenticate message. However, it has to be sent before transmitting any audio data. | ||
| // - The server will reply with a Capabilities message as well, with one item in the list, | ||
| // indicating the protocol to be used. | ||
| // - In the middle of a session, a client can perform this negotiation again by sending another | ||
| // valid Capabilities message. The server may also request the client to use another protocol | ||
| // by sending a Capabilities message with the protocol it'd like to switch to. | ||
| message Capabilities { |
There was a problem hiding this comment.
I think either the name of this message type or its description is misleading. The description is tailored to a specific voice protocol negotiation, but the name Capabilities seems to imply a more general use-case.
For instance, I would assume that a Capabilities message would also contain information about whether or not the client knows the new channel listener feature, which audio codec it knows to use, etc.
And in fact, I think this all makes sense to be bundled together with the encryption methods the client knows. Thus, I think the description should be generalized a bit.
There was a problem hiding this comment.
The description is tailored to a specific voice protocol negotiation, but the name
Capabilitiesseems to imply a more general use-case.
IIRC the original idea was to create a general message type, where people can add additional feature flags into this message. I also recalled I wrote in another issue that the codec information is dispersed among many message types (but in the end I think people felt we should mark them as obsolete because Opus will be the only option).
The description might seem to be a little bit narrow. I can redo the description. But do we really want to add more items to this message in the future? If not, I can consider renaming it into something like ProtocolCapabilities.
There was a problem hiding this comment.
But do we really want to add more items to this message in the future? If not, I can consider renaming it into something like ProtocolCapabilities.
I would think so, yes. So in general I was thinking that this could really contain flags for all the different features the Mumble protocol supports in principle and then clients and servers can specify which of these they do in fact support.
| // Currently available options: | ||
| // - MUMBLE_UDP_AES-128-OCB2 | ||
| // - MUMBLE_UDP_AES-256-GCM | ||
| repeated string protocols = 1; |
There was a problem hiding this comment.
In light of the above comment, I think naming this audio_protocols might be more precise and could avoid some confusion in the future.
| explicit VoiceProtocol(const std::string &s); | ||
|
|
||
| public: | ||
| VoiceTransportType m_transport; |
There was a problem hiding this comment.
In general, I don't like public member fields, as that means they can be modified from the outside, without the class's knowledge.
Any particular reason, you chose to make this member public instead of making it private/protected and adding public getter/setter methods?
There was a problem hiding this comment.
I think getter is a good idea. WIll do.
| virtual bool operator==(const VoiceProtocol &rhs) const; | ||
| virtual std::string toString() const; | ||
| static std::shared_ptr< VoiceProtocol > fromString(const std::string &s); | ||
| bool isValid() const; | ||
| VoiceProtocol(); | ||
| virtual ~VoiceProtocol(){}; |
There was a problem hiding this comment.
Imo ordering the declarations in this way, allows a quicker overview of the class.
| virtual bool operator==(const VoiceProtocol &rhs) const; | |
| virtual std::string toString() const; | |
| static std::shared_ptr< VoiceProtocol > fromString(const std::string &s); | |
| bool isValid() const; | |
| VoiceProtocol(); | |
| virtual ~VoiceProtocol(){}; | |
| static std::shared_ptr< VoiceProtocol > fromString(const std::string &s); | |
| VoiceProtocol(); | |
| virtual ~VoiceProtocol() = default; | |
| virtual std::string toString() const; | |
| bool isValid() const; | |
| virtual bool operator==(const VoiceProtocol &rhs) const; |
| class UDPVoiceProtocol : public VoiceProtocol { | ||
| private: | ||
| explicit UDPVoiceProtocol(const std::string &s); | ||
| friend VoiceProtocol; |
There was a problem hiding this comment.
Why is this needed? The class already inherits VoiceProtocol, so one could assume making them friends is unnecessary? 🤔
There was a problem hiding this comment.
I think the problem is I want to call UDPVoiceProtocol's function in VoiceProtocol... See
Lines 21 to 24 in d20d3e9
I know it doesn't feel right. And actually my solution to similar problems (see the crypto part) was to create another factory object to do the conversion... Maybe I should do it here as well?
There was a problem hiding this comment.
Ah so you are calling a private constructor. Okay then that makes sense then 💡
Note however that you are misusing make_shared (slightly). It should look like this:
return std::make_shared< UDPVoiceProtocol >(s);The way you wrote it, it first creates an instance of the class and then copies (or moves) that to another instance which is then contained in the shared pointer. With my version you are only creating a single object and don't copy/move a second one around :)
| } | ||
|
|
||
| UDPVoiceProtocol::UDPVoiceProtocol(const std::string &s) : VoiceProtocol(s) { | ||
| if (s.rfind("MUMBLE_UDP_", 0) != 0) |
|
|
||
| m_transport = VoiceTransportType::UDP; | ||
|
|
||
| std::string cipher_name = s.substr(strlen("MUMBLE_UDP_")); |
There was a problem hiding this comment.
| std::string cipher_name = s.substr(strlen("MUMBLE_UDP_")); | |
| std::string cipher_name = s.substr(std::strlen("MUMBLE_UDP_")); |
(Just a bit more C++-ish instead of C-ish)
| m_valid = true; | ||
| m_transport = VoiceTransportType::UDP; | ||
| m_udpCipher = cipher; | ||
| m_protocolName = "MUMBLE_UDP_" + std::string(CipherTypeDescription(cipher).name); |
There was a problem hiding this comment.
Shouldn't this be passed to the VoiceProtocol constructor instead of the empty string? It seems that that way you would not have to set this member variable here explicitly 🤔
There was a problem hiding this comment.
Yeah will try. I can't see the reason I have done this in this strange way as well.
| UDPVoiceProtocol::UDPVoiceProtocol(CipherType cipher) : VoiceProtocol("") { | ||
| m_valid = true; | ||
| m_transport = VoiceTransportType::UDP; | ||
| m_udpCipher = cipher; |
There was a problem hiding this comment.
initializer lists should be preferred:
| UDPVoiceProtocol::UDPVoiceProtocol(CipherType cipher) : VoiceProtocol("") { | |
| m_valid = true; | |
| m_transport = VoiceTransportType::UDP; | |
| m_udpCipher = cipher; | |
| UDPVoiceProtocol::UDPVoiceProtocol(CipherType cipher) : VoiceProtocol(""), m_valid(true), m_transport(VoiceTransportType::UDP), m_udpCipher(cipher) { |
| // that can be found in the LICENSE file at the root of the | ||
| // Mumble source tree or at <https://www.mumble.info/LICENSE>. | ||
|
|
||
| #ifndef MUMBLE_VOICEPROTOCOLTYPE_H |
There was a problem hiding this comment.
Include-guard does not (exactly) match file-name
There was a problem hiding this comment.
I must be lost during numerous refactors...
|
@Krzmbrzl Great to know you are back again! I'll do my best to find some time to work on this PR in the coming few weeks. |
|
libmumble provides two classes:
I would make use of those before completing the pull request. By the way, identifying ciphers using their official name like OpenSSL does would be ideal: https://github.com/openssl/openssl/blob/1751356267f64d5db8824cf4ff5b3496e15972da/crypto/objects/objects.txt |
|
@davidebeatrici Yeah, actually I paused working on this PR because of additional OpenSSL issues we were experiencing and the fact that people were busy rolling out 1.4.0. I think it is a good time now to restart it. One question: What is the roadmap of migrating the current Mumble implementation into libmumble? Do we have a specific branch for that task? If there's one, then I would base this PR onto that branch. I also think I should code AES-256-GCM and ChaCha into libmumble. I think I would open a new PR there. |
|
I would argue that we should hold on for this PR a bit longer until we have figured out how we want to go about using libmumble and when we want to start doing so. Davide did not only keep his work secret to the public but also internally (we knew that he was working on it but we couldn't look at the code either). |
|
Okay, since Davide consistently refuses to add documentation to libmumble, it will not be integrated into main Mumble anytime soon. Therefore, I think it'd still make sense to finish this PR here. @TerryGeng do you still have motivation to continue with this? |
|
I never refused the task, it's in my TODO list. |
|
Doesn't change the current situation though |
|
sry but - any news on this?! |
This PR is a continuation of #4299. It implements the ideas from #4238, #3918, #4248, #4719.
Capabilities Message
In order to negotiate the voice transport protocol between clients and servers, a new message
Capabilitieswas created.Inside this message, a string field
supported_protocolsis used to indicate protocols the client supports, while the server replies with aCapabilitiesmessage with only one item as the protocol it determines to use in the future voice transport.Tunneling through TCP isn't listed as an option here because it is assumed that all clients have implemented this function.
This negotiation will be initiated by the client, right before sending the
Authenticatemessage. After this process is completed and theAuthenticatemessage is sent, the server sendsCryptSetupmessage to complete the cipher sync and then two parties start to transmit voice packets through UDP channels.The usage of string inside the
Capabilitiesmessage incited a fierce discussion in #4299. Some people suggested using an enum plus a string field to hold both official protocols and 3rd-party protocols, citing the potential trouble of typo. However, it seems that this design exacerbates the complexity of the overall design. In light of the discussion in #4299 and #4719, I decided to go with the pure string design and converting them into an enum after received by the server/client.The direct usage of strings to indicate protocols was intentionally avoided to avoid typos. Instead, the
toStringmethod ofVoiceProtocolwas used when strings are needed.A server config option called "voiceProtocolPreference" was also added. One can list the preference of the server like
UDP_AES_256_GCM,UDP_AES_128_OCB2, sorted by priority.AES-256-GCM
The addition of AES-256-GCM from OpenSSL mainly serves as a proof of concept (and is the preferred cipher by default in my implementation). According to the discussion in #3918. The reason for selecting AES-256-GCM is because I'm familiar with it and it is one candidate listed in #3918. Also, OpenSSL started to support AES-256-GCM from 1.0.1 so I think our binary can be linked on most platforms. However, checking the availability of EVP functions wasn't implemented and I'm not sure if it is necessary. Meanwhile, OpenSSL provides very good hardware acceleration on some platforms with AES instruction set implemented.
I tried my best to follow the guidance from NIST: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
Useful information about EVP can also be found at https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption
I'd like to thank @Krzmbrzl, @davidebeatrici, and @Johni0702. These nice people provided very useful insights and showed great patience in the discussion (even I constantly act like an idiot). This PR won't come into being without their precious help.
Suggestion for testers
Checks
ToDo:
updatetranslations.pydoesn't detect translation changes in subdirectories (src/crypto/CipherType.h)