Voice transport negotiation and AES-256-GCM #4824

TerryGeng · 2021-03-04T08:51:58Z

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 Capabilities was created.
Inside this message, a string field supported_protocols is used to indicate protocols the client supports, while the server replies with a Capabilities message 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 Authenticate message. After this process is completed and the Authenticate message is sent, the server sends CryptSetup message to complete the cipher sync and then two parties start to transmit voice packets through UDP channels.

The usage of string inside the Capabilities message 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 toString method of VoiceProtocol was 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

Check if backward compatibility is preserved. Connect old clients to new servers and new clients to old servers and observe if it works. (I have tested, it works)

Checks

My commits follow the commit guidelines
I have created test cases to ensure my code works.

ToDo:

Figure out why updatetranslations.py doesn't detect translation changes in subdirectories (src/crypto/CipherType.h)

Avatat

Great job!

src/Message.h

src/crypto/CryptState.h

src/Mumble.proto

davidebeatrici

Mostly cosmetic stuff.

src/Connection.h

src/Message.h

src/Mumble.proto

src/murmur/Messages.cpp

src/murmur/ServerUser.h

src/tests/TestAES256GCMCrypt/CMakeLists.txt

src/tests/TestAES256GCMCrypt/TestAES256GCMCrypt.cpp

davidebeatrici · 2021-03-04T09:27:57Z

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.

TerryGeng · 2021-03-04T09:45:00Z

@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.

Krzmbrzl

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 of unsigned 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

src/Connection.cpp

src/Connection.h

src/Mumble.proto

src/murmur/Messages.cpp

src/murmur/Meta.cpp

src/murmur/Meta.h

src/murmur/Server.h

src/murmur/ServerUser.h

TerryGeng · 2021-03-08T01:07:42Z

@Krzmbrzl Thanks for the thorough review :)

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.

Make sense.

In general I think using uint32_t (and its friends) are the more modern (better?) way of using unsigned integer types (aka: instead of unsigned int)

Make sense.

A little more documentation on the new classes could be useful

Make sense.

Please drop the translation commit, rebase against current master and then use the new python script to update the translations

I had an old problem with using the translation script, as lupdate will always create some duplicated items (perhaps my Qt version is buggy), not sure why. So the translation commits you have seen here were actually manually done based on the reaction of the translation CI... Can't find a better way out. I will try to upgrade again to see if this problem has been solved.

Sorry for the myriad of review comments xD

No worries :) I will carefully read them one by one.

Krzmbrzl · 2021-03-08T08:55:19Z

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.
Thus I think keeping a fixed (small) list is actually better for us.

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 inofficial in the UI. This way we can make sure that experienced users can use all the ciphers they want to (having to deal with potential problems themselves), but novice users (hopefully) won't touch them.

TerryGeng · 2021-03-12T15:18:10Z

Regarding why I gave up adding TCP_TUNNELING into the voice transport list:

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 TCP_TUNNELING.

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 bUdp in src/mumble/ServerHandler.cpp). However, even after switched back to TCP mode, the client will keep sending UDP ping and resume the UDP connection after ping can be received again.

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 TCP_TUNNELING mode as a secondary option, and the server initialize both options and waiting for data from both channels. However, this only works for one TCP + one UDP since I can't place two UDP transports here because the server will feel confused about which cipher to use to decrypt the incoming packet. Then I thought: why make such a fuss? I can just assume the TCP channel is always there as a part of the control protocol.

Krzmbrzl · 2021-03-12T16:54:41Z

I can just assume the TCP channel is always there as a part of the control protocol.

I agree 👍
I remember there being a TODO comment in the code about that option though... Was this placed for this exact reason with the intention of potentially figuring stuff out later (or alternatively dropping the thing)?

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 official_ or mumble_ or any other unique prefix that separate the officially supported protocols from custom ones. I don't recall having seen something like this during my review. Did you do it and I missed it or did you forget about it (or left it out intentionally)? 🤔

TerryGeng · 2021-03-13T07:59:30Z

I remember there being a TODO comment in the code about that option though... Was this placed for this exact reason with the intention of potentially figuring stuff out later (or alternatively dropping the thing)?

🤔️ Wondering which TODO do you mean.
https://github.com/mumble-voip/mumble/pull/4824/files#diff-f0a843499abea1ccb70ca40e66cacd955b3fdd25b0a1cd65ff28ccb961536881R2159 ⬅️️ This one only applies to UserStats messages. I think it makes no sense to send UDP statistics when the user is on TCP tunneling mode. Also, the statistics are saved in CryptState, which may not exist if that user is on TCP mode.

start with official_ or mumble_ or any other unique prefix

If you mean the protocol name string, I didn't add such prefixes. Do you mean something like MUMBLE_UDP_AES_128_GCM?

Krzmbrzl · 2021-03-13T09:44:31Z

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.

TredwellGit · 2021-03-13T19:27:12Z

src/crypto/CryptStateAES256GCM.cpp

+void CryptStateAES256GCM::genKey() {
+	RAND_bytes(raw_key, keyLength);
+	RAND_bytes(encrypt_iv, ivLength);
+	RAND_bytes(decrypt_iv, ivLength);
+	bInit = true;
+}


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.

@Krzmbrzl, @davidebeatrici, the OCB2 implementation does this which can be a security issue:

mumble/src/crypto/CryptStateOCB2.cpp

Lines 44 to 51 in f5177b8

void CryptStateOCB2::genKey() {

RAND_bytes(raw_key, AES_KEY_SIZE_BYTES);

RAND_bytes(encrypt_iv, AES_BLOCK_SIZE);

RAND_bytes(decrypt_iv, AES_BLOCK_SIZE);

AES_set_encrypt_key(raw_key, AES_KEY_SIZE_BITS, &encrypt_key);

AES_set_decrypt_key(raw_key, AES_KEY_SIZE_BITS, &decrypt_key);

bInit = true;

}

There is this but the implementation is bad:

mumble/src/crypto/CryptographicRandom.cpp

Lines 14 to 31 in f5177b8

void CryptographicRandom::fillBuffer(void *buf, int numBytes) {

// We treat negative and zero values of numBytes to be

// fatal errors in the program. Abort the program if such

// a value is passed to fillBuffer().

if (numBytes <= 0) {

qFatal("CryptographicRandom::fillBuffer(): numBytes is <= 0");

}

// RAND_bytes only returns an error if the entropy pool has not yet been sufficiently filled,

// or in the case of a catastrophic, unrecoverable error in the RAND_bytes implementation happens.

// OpenSSL needs at least 32-bytes of high-entropy random data to seed its CSPRNG.

// If OpenSSL cannot acquire enough random data to seed its CSPRNG at the time Mumble and Murmur

// are running, there is not much we can do about it other than aborting the program.

if (RAND_bytes(reinterpret_cast< unsigned char * >(buf), static_cast< int >(numBytes)) != 1) {

qFatal("CryptographicRandom::fillBuffer(): internal error in OpenSSL's RAND_bytes or entropy pool not yet "

"filled.");

}

}

Consider using https://doc.libsodium.org/generating_random_data.

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?

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.

Alright. Thanks for the clarification 👍

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.

TerryGeng · 2021-03-29T03:09:05Z

Memo:

Turn the protocols field in Mumble.proto into a list of Transport structs with two fields protocol and cipher (in case that in the future we added WebRTC into the list) (only UDP transport requires all encryptions to be handled by Mumble and I need a way to filter these UDP transports out) ⬅️️ Do you think this is okay? @Krzmbrzl
Prepend protocols and ciphers with the prefix MUMBLE_ to indicate it is the official one.
Create a global object (CryptStateFactory) to handle UDP cipher list (it will handle the cipher field in the Transport struct directly and return a CryptState object accordingly so that VoiceProtocolType enum won't be flying around all over the codebase)
Use std containers instead of containers from Qt and use the new naming convention.
Add Chacha.

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.

Krzmbrzl · 2021-03-30T07:15:40Z

urn the protocols field in Mumble.proto into a list of Transport structs with two fields protocol and cipher (in case that in the future we added WebRTC into the list) (only UDP transport requires all encryptions to be handled by Mumble and I need a way to filter these UDP transports out) arrow_left️ Do you think this is okay? Krzmbrzl

So for the time being protocol would always be "UDP" and then cipher would be what is currently sent around in these messages, right?
You mentioned the example WebRTC that (to my impression) does not require the cipher field at all. If we send a struct as you proposed, then we'd always have to send the dead weight of a dummy cipher with it. This seems like it could be improved 🤔

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.
With Strings we could make the String follow a specific pattern (e.g. (Official_)?<protocol>(_<cipher>)?) but I'm not sure if the source of errors here is a bit too high...

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.

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 ☝️

TerryGeng · 2021-03-30T07:25:08Z

You mentioned the example WebRTC that (to my impression) does not require the cipher field at all. If we send a struct as you proposed, then we'd always have to send the dead weight of a dummy cipher with it. This seems like it could be improved

I have an impression that an optional field won't be transmitted if left empty (and given that the Capabilities message will only be sent during the negotiation so the overhead should be fine though).

You'll have to do the rebasing eventually anyways, since we want the commit history to be clean

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.

your list is missing the prefix for the String representation of officially supported protocols/ciphers

Added :)

Krzmbrzl · 2021-03-30T13:12:58Z

I have an impression that an optional field won't be transmitted

Yes that is the case. Can fields in a struct be optional? In that case don't mind my comment above :D

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.

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.

Krzmbrzl · 2021-06-15T07:16:40Z

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?

TredwellGit · 2021-06-15T07:22:57Z

#4219 (comment)
#3918 (comment)
#3918 (comment)

davidebeatrici · 2021-06-15T07:54:57Z

@jedisct1 expressed concerns about it in #3918 (comment).

Krzmbrzl · 2021-06-15T10:33:53Z

Ho - guess I have not followed that discussion that attentively xD
-> #3918 (comment)

TerryGeng · 2021-06-16T01:22:10Z

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?

Krzmbrzl · 2021-06-16T06:15:16Z

is there any update on this matter?

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 :)

Should I continue or wait for the result of the new discussion of DTLS in #3918?

I would at least wait a bit to see where the discussion leads 🤔

Krzmbrzl · 2021-07-01T06:13:29Z

@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 :)

TerryGeng · 2021-07-04T08:41:59Z

@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.

TerryGeng · 2021-07-20T07:28:57Z

src/crypto/CryptStateEVP.h

+	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];


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.

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.

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.

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?

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.

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).

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.

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?

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.

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? 👀

src/crypto/CryptStateEVP.h

src/crypto/CryptState.h

src/VoiceProtocol.h

TerryGeng · 2021-07-20T15:10:32Z

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 CryptStateEVP interface created opened a way to quickly add any new ciphers from the EVP.

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.

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.

yanmaani · 2021-11-03T18:44:25Z

src/crypto/CryptStateEVP.h

+
+	bool isValid() const override;
+	void genKey() override;
+	bool setKey(const std::string &rkey, const std::string &eiv, const std::string &div) override;


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?

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

// Regarding GCM, see NIST Special Publication 800-38D

// https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf

static const EVP_CIPHER *cipher;

// IV generation procedure is detailed in NIST 800-38D Sec. 8.2

// The construction we adopt is Deterministic Construction in 8.2.1

// 32-bit of IV is fixed field and the remaining 64-bit is

// "invocation field", which is an integer counter.

static constexpr unsigned int IV_LENGTH = 96 / 8;

Oh, so there's a separate API exposed somewhere that just takes the nonce and does the rest for us, like NaCl does?

Yes. It is combined in the init function.

mumble/src/crypto/CryptStateEVP.h

Lines 138 to 139 in d20d3e9

if (1 != EVP_EncryptInit(ctx, cipher, raw_key, encrypt_iv))

return false;

Krzmbrzl

Okay so I am back from my hibernation. I hope that I will find the necessary time to finally get this PR merged. For now, let's start with this partial review. I will continue with it as I find the time :)

Krzmbrzl · 2021-11-05T18:54:00Z

src/Mumble.proto

+// 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 {


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.

The description is tailored to a specific voice protocol negotiation, but the name Capabilities seems 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.

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.

Krzmbrzl · 2021-11-05T18:55:06Z

src/Mumble.proto

+  // Currently available options:
+  //  - MUMBLE_UDP_AES-128-OCB2
+  //  - MUMBLE_UDP_AES-256-GCM
+  repeated string protocols = 1;


In light of the above comment, I think naming this audio_protocols might be more precise and could avoid some confusion in the future.

Krzmbrzl · 2021-11-05T18:57:56Z

src/VoiceProtocol.h

+	explicit VoiceProtocol(const std::string &s);
+
+public:
+	VoiceTransportType m_transport;


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?

I think getter is a good idea. WIll do.

Krzmbrzl · 2021-11-05T19:01:23Z

src/VoiceProtocol.h

+	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(){};


Imo ordering the declarations in this way, allows a quicker overview of the class.

Suggested change

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;

Krzmbrzl · 2021-11-05T19:02:15Z

src/VoiceProtocol.h

+class UDPVoiceProtocol : public VoiceProtocol {
+private:
+	explicit UDPVoiceProtocol(const std::string &s);
+	friend VoiceProtocol;


Why is this needed? The class already inherits VoiceProtocol, so one could assume making them friends is unnecessary? 🤔

I think the problem is I want to call UDPVoiceProtocol's function in VoiceProtocol... See

mumble/src/VoiceProtocol.cpp

Lines 21 to 24 in d20d3e9

std::shared_ptr< VoiceProtocol > VoiceProtocol::fromString(const std::string &s) {

if (s.rfind("MUMBLE_UDP_", 0) != std::string::npos)

return std::make_shared< UDPVoiceProtocol >(UDPVoiceProtocol(s));

else {

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?

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 :)

Krzmbrzl · 2021-11-05T19:09:16Z

src/VoiceProtocol.cpp

+}
+
+UDPVoiceProtocol::UDPVoiceProtocol(const std::string &s) : VoiceProtocol(s) {
+	if (s.rfind("MUMBLE_UDP_", 0) != 0)


Why rfind instead of find?

Krzmbrzl · 2021-11-05T19:09:42Z

src/VoiceProtocol.cpp

+
+	m_transport = VoiceTransportType::UDP;
+
+	std::string cipher_name = s.substr(strlen("MUMBLE_UDP_"));


Suggested change

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)

Krzmbrzl · 2021-11-05T19:11:34Z

src/VoiceProtocol.cpp

+	m_valid        = true;
+	m_transport    = VoiceTransportType::UDP;
+	m_udpCipher    = cipher;
+	m_protocolName = "MUMBLE_UDP_" + std::string(CipherTypeDescription(cipher).name);


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 🤔

Yeah will try. I can't see the reason I have done this in this strange way as well.

Krzmbrzl · 2021-11-05T19:12:35Z

src/VoiceProtocol.cpp

+UDPVoiceProtocol::UDPVoiceProtocol(CipherType cipher) : VoiceProtocol("") {
+	m_valid        = true;
+	m_transport    = VoiceTransportType::UDP;
+	m_udpCipher    = cipher;


initializer lists should be preferred:

Suggested change

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) {

Krzmbrzl · 2021-11-05T19:13:04Z

src/VoiceProtocol.h

+// 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


Include-guard does not (exactly) match file-name

I must be lost during numerous refactors...

TerryGeng · 2021-11-07T15:44:14Z

@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.

davidebeatrici · 2022-02-01T08:32:07Z

libmumble provides two classes:

Crypt, which supports all ciphers provided by OpenSSL.
CryptOCB2, which supports our AES-128-OCB implementation.

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

TerryGeng · 2022-02-01T13:54:59Z

@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.

Krzmbrzl · 2022-02-01T14:04:50Z

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).
I have the feeling that depending on the scope of libmumble, this PR might better be implemented inside libmumble itself

Krzmbrzl · 2022-09-09T08:07:19Z

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?

davidebeatrici · 2022-09-09T08:33:34Z

I never refused the task, it's in my TODO list.

Krzmbrzl · 2022-09-09T08:35:17Z

Doesn't change the current situation though

fred0r · 2022-12-07T03:52:07Z

sry but - any news on this?!

Avatat reviewed Mar 4, 2021

View reviewed changes

src/Message.h Outdated Show resolved Hide resolved

src/crypto/CryptState.h Outdated Show resolved Hide resolved

TerryGeng commented Mar 4, 2021

View reviewed changes

src/Mumble.proto Show resolved Hide resolved

davidebeatrici reviewed Mar 4, 2021

View reviewed changes

TerryGeng force-pushed the capabilities branch from b69698f to 757d443 Compare March 4, 2021 10:14

Krzmbrzl requested changes Mar 7, 2021

View reviewed changes

TredwellGit suggested changes Mar 13, 2021

View reviewed changes

Avatat mentioned this pull request May 26, 2021

Scaling murmur server. #5042

Closed

TerryGeng added 2 commits July 13, 2021 15:00

Krzmbrzl added the feature-request This issue or PR deals with a new feature label Jul 19, 2021

TerryGeng commented Jul 20, 2021

View reviewed changes

src/crypto/CryptStateEVP.h Outdated Show resolved Hide resolved

TerryGeng commented Jul 20, 2021

View reviewed changes

src/crypto/CryptState.h Show resolved Hide resolved

TerryGeng force-pushed the capabilities branch 2 times, most recently from 5fc583c to 7f70075 Compare July 20, 2021 08:05

TerryGeng commented Jul 20, 2021

View reviewed changes

src/VoiceProtocol.h Outdated Show resolved Hide resolved

TerryGeng force-pushed the capabilities branch from 7f70075 to b7cb1e1 Compare July 21, 2021 07:27

TerryGeng force-pushed the capabilities branch 5 times, most recently from ab4e9e1 to 234489c Compare July 22, 2021 01:48

FEAT(cipher): Generalize EVP interface, add CHACHA20-POLY1305.

d20d3e9

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.

TerryGeng force-pushed the capabilities branch from 234489c to d20d3e9 Compare July 22, 2021 01:53

yanmaani reviewed Nov 3, 2021

View reviewed changes

Krzmbrzl reviewed Nov 5, 2021

View reviewed changes

TerryGeng mentioned this pull request Dec 7, 2021

BUILD(crypto): Migrate to OpenSSL 3.0 API #5352

Merged

1 task

TerryGeng mentioned this pull request Jan 18, 2022

Can't hear anyone in mumble after 5-10 minutes #5421

Closed

Krzmbrzl mentioned this pull request Feb 7, 2022

Change encryption algorithm #5559

Closed

Krzmbrzl marked this pull request as draft January 1, 2024 19:01

	void CryptStateOCB2::genKey() {
	RAND_bytes(raw_key, AES_KEY_SIZE_BYTES);
	RAND_bytes(encrypt_iv, AES_BLOCK_SIZE);
	RAND_bytes(decrypt_iv, AES_BLOCK_SIZE);
	AES_set_encrypt_key(raw_key, AES_KEY_SIZE_BITS, &encrypt_key);
	AES_set_decrypt_key(raw_key, AES_KEY_SIZE_BITS, &decrypt_key);
	bInit = true;
	}

	void CryptographicRandom::fillBuffer(void *buf, int numBytes) {
	// We treat negative and zero values of numBytes to be
	// fatal errors in the program. Abort the program if such
	// a value is passed to fillBuffer().
	if (numBytes <= 0) {
	qFatal("CryptographicRandom::fillBuffer(): numBytes is <= 0");
	}

	// RAND_bytes only returns an error if the entropy pool has not yet been sufficiently filled,
	// or in the case of a catastrophic, unrecoverable error in the RAND_bytes implementation happens.
	// OpenSSL needs at least 32-bytes of high-entropy random data to seed its CSPRNG.
	// If OpenSSL cannot acquire enough random data to seed its CSPRNG at the time Mumble and Murmur
	// are running, there is not much we can do about it other than aborting the program.
	if (RAND_bytes(reinterpret_cast< unsigned char * >(buf), static_cast< int >(numBytes)) != 1) {
	qFatal("CryptographicRandom::fillBuffer(): internal error in OpenSSL's RAND_bytes or entropy pool not yet "
	"filled.");
	}
	}

	// Regarding GCM, see NIST Special Publication 800-38D
	// https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf

	static const EVP_CIPHER *cipher;

	// IV generation procedure is detailed in NIST 800-38D Sec. 8.2
	// The construction we adopt is Deterministic Construction in 8.2.1
	// 32-bit of IV is fixed field and the remaining 64-bit is
	// "invocation field", which is an integer counter.

	static constexpr unsigned int IV_LENGTH = 96 / 8;

	if (1 != EVP_EncryptInit(ctx, cipher, raw_key, encrypt_iv))
	return false;

	std::shared_ptr< VoiceProtocol > VoiceProtocol::fromString(const std::string &s) {
	if (s.rfind("MUMBLE_UDP_", 0) != std::string::npos)
	return std::make_shared< UDPVoiceProtocol >(UDPVoiceProtocol(s));
	else {


		m_transport = VoiceTransportType::UDP;

		std::string cipher_name = s.substr(strlen("MUMBLE_UDP_"));

	std::string cipher_name = s.substr(strlen("MUMBLE_UDP_"));
	std::string cipher_name = s.substr(std::strlen("MUMBLE_UDP_"));

Voice transport negotiation and AES-256-GCM #4824

Are you sure you want to change the base?

Voice transport negotiation and AES-256-GCM #4824

Conversation

TerryGeng commented Mar 4, 2021 • edited by Krzmbrzl

Capabilities Message

AES-256-GCM

Suggestion for testers

Checks

Avatat left a comment

Choose a reason for hiding this comment

davidebeatrici left a comment

Choose a reason for hiding this comment

davidebeatrici commented Mar 4, 2021

TerryGeng commented Mar 4, 2021

Krzmbrzl left a comment • edited

Choose a reason for hiding this comment

TerryGeng commented Mar 8, 2021

Krzmbrzl commented Mar 8, 2021

TerryGeng commented Mar 12, 2021

Krzmbrzl commented Mar 12, 2021

TerryGeng commented Mar 13, 2021

Krzmbrzl commented Mar 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng commented Mar 29, 2021 • edited

Krzmbrzl commented Mar 30, 2021

TerryGeng commented Mar 30, 2021

Krzmbrzl commented Mar 30, 2021

Krzmbrzl commented Jun 15, 2021 • edited

TredwellGit commented Jun 15, 2021

davidebeatrici commented Jun 15, 2021

Krzmbrzl commented Jun 15, 2021

TerryGeng commented Jun 16, 2021

Krzmbrzl commented Jun 16, 2021

Krzmbrzl commented Jul 1, 2021

TerryGeng commented Jul 4, 2021

Choose a reason for hiding this comment

TerryGeng Jul 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng Jul 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng commented Jul 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng Nov 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng commented Mar 4, 2021 •

edited by Krzmbrzl

Krzmbrzl left a comment •

edited

TerryGeng commented Mar 29, 2021 •

edited

Krzmbrzl commented Jun 15, 2021 •

edited

TerryGeng Jul 20, 2021 •

edited

TerryGeng Jul 22, 2021 •

edited

TerryGeng commented Jul 20, 2021 •

edited

TerryGeng Nov 10, 2021 •

edited