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

FIX(ocb2): Work around packet loss due to OCB2 XEX* mitigation #4720

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

Johni0702
Copy link
Contributor

The mitigation for vulnerabilities discovered in
OCB2 (https://eprint.iacr.org/2019/311, called XEX* attack, or XEXStarAttack, in code)
introduced in be97594 (#4227) willingly allowed for some packets with specific
characteristics to be dropped during encryption to prevent the vulnerability
from being exploited.
It was assumed that the chance of such packets was sufficiently small (given
we are dealing with compressed audio) that such loss was acceptable.

It was however discovered that digital silence (as produced by e.g. a noise
gate) will cause Opus to emit almost exclusively such packets, leading to strong
artifacts on the receiving end. See #4385.

This commit tries to work around the issue by modifying such packets in a way
which will no longer require them to be dropped, and yet produce the expected
output on the receiver side.
As far as I understand Opus (specifically section 4.1, 4.3.0 and 4.3.3), the
0s are simply unused bits and are only there because we running Opus in constant
bitrate mode. So, flipping one of them should have no effect on the resulting
audio.

Fixes #4719

@Johni0702
Copy link
Contributor Author

I don't know enough C++ to understand why this fails to compile/link on Ubuntu 18 nor how to fix that.

@TredwellGit
Copy link
Contributor

Fixes packet loss and sounds fine.

@TredwellGit
Copy link
Contributor

Also works without needing to update the server.

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but preferably I would have a few more people report that this does not affect the audio on their system before merging this.

src/tests/TestCrypt/TestCrypt.cpp Outdated Show resolved Hide resolved
@Krzmbrzl Krzmbrzl added backport-needed bug A bug (error) in the software labels Jan 27, 2021
@MartB
Copy link

MartB commented Jan 28, 2021

LGTM but preferably I would have a few more people report that this does not affect the audio on their system before merging this.

It certainly doesnt make it worse than it already is^^
Count this as feedback from 2 people that the "packet loss" is considerably better after testing this version.

@davidebeatrici
Copy link
Member

Do you mean that you still experience packet loss?

@MartB
Copy link

MartB commented Jan 28, 2021

Do you mean that you still experience packet loss?

Kinda, im not sure what its coming from tho, so it might be out of scope of this PR.
Its on incoming audio streams and it somehow sounds "robotic" just like packet loss but theres no packet loss indicated by mumble. (Everyone is randomly affected) (testing on windows atm)

This goes away when using v1.3

Im going to try and just get rid of the security fix for the encryption and see if the issue goes away completely.

It mostly appears the moment someone starts speaking after a silence and for longer transmissions it seems to be fine.

@TerryGeng
Copy link
Contributor

TerryGeng commented Feb 3, 2021

This fix should work for blank packets in principle. I tested the opus. Feeding in 240 zeros gives

b'\xe8\xff\xfe'

While modified this into

b'\xe8\xff\xfe\x00\x00\x00\x01'

and decoded returns with 240 zeros.

It turns out that as long as the first three bytes don't change, it will always return zeros. One can even append garbage after it.

@TerryGeng
Copy link
Contributor

@MartB This depends on your testing setup. You need to have all parties to be fixed by this PR (or one is fixed by this PR and others are all below 1.3.3) to see if this PR actually works. Any 1.3.3 clients or servers in the middle will meddle up the result.

@Johni0702
Copy link
Contributor Author

Johni0702 commented Feb 3, 2021

Any 1.3.3 clients or servers in the middle will meddle up the result.

It should actually be sufficient for just the original sender to have the fix because the moment it sends packets which would have been filtered by 1.3.3, it instead modifies those packets in such a way that they will no longer be filtered, so the version of anything beyond that point shouldn't have any influence on that packet.

Except if the original sender is using TCP (or some future protocol) for voice, then the server needs it as well.

@MartB
Copy link

MartB commented Feb 3, 2021

@MartB This depends on your testing setup. You need to have all parties to be fixed by this PR (or one is fixed by this PR and others are all below 1.3.3) to see if this PR actually works. Any 1.3.3 clients or servers in the middle will meddle up the result.

Yeah we were using the azure ci builds of this PR.
Im still not sure whats actually going on, if i find the time to test i will try out different configurations with my buddies.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Feb 4, 2021

From what I have seen in the results reported here this PR seems to fix the issue at hand. In the worst case it does not remove all packet loss but at least improves it significantly. Thus I would say that we can prepare this PR for being merged.

Any objections?
@davidebeatrici

The mitigation for vulnerabilities discovered in
OCB2 (https://eprint.iacr.org/2019/311, called XEX* attack, or XEXStarAttack, in code)
introduced in be97594 (mumble-voip#4227) willingly allowed for some packets with specific
characteristics to be dropped during encryption to prevent the vulnerability
from being exploited.
It was assumed that the chance of such packets was sufficiently small (given
we are dealing with compressed audio) that such loss was acceptable.

It was however discovered that digital silence (as produced by e.g. a noise
gate) will cause Opus to emit almost exclusively such packets, leading to strong
artifacts on the receiving end. See mumble-voip#4385.

This commit tries to work around the issue by modifying such packets in a way
which will no longer require them to be dropped, and yet produce the expected
output on the receiver side.
As far as I understand [Opus] (specifically section 4.1, 4.3.0 and 4.3.3), the
0s are simply unused bits and are only there because we running Opus in constant
bitrate mode. So, flipping one of them should have no effect on the resulting
audio.

[Opus]: https://tools.ietf.org/html/rfc6716

Fixes mumble-voip#4719
@Krzmbrzl Krzmbrzl merged commit b635342 into mumble-voip:master Feb 5, 2021
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Feb 5, 2021

Thank you very much for this fix! 👍

@TredwellGit
Copy link
Contributor

#4417 should be closed.

@TredwellGit
Copy link
Contributor

Please release 1.3.4 with this and #4733.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Feb 6, 2021

Please release 1.3.4 with this and #4733.

That's the plan :)

Krzmbrzl added a commit that referenced this pull request Feb 6, 2021
…s due to OCB2 XEX* mitigation"

The mitigation for vulnerabilities discovered in
OCB2 (https://eprint.iacr.org/2019/311, called XEX* attack, or XEXStarAttack, in code)
introduced in be97594 (#4227) willingly allowed for some packets with specific
characteristics to be dropped during encryption to prevent the vulnerability
from being exploited.
It was assumed that the chance of such packets was sufficiently small (given
we are dealing with compressed audio) that such loss was acceptable.

It was however discovered that digital silence (as produced by e.g. a noise
gate) will cause Opus to emit almost exclusively such packets, leading to strong
artifacts on the receiving end. See #4385.

This commit tries to work around the issue by modifying such packets in a way
which will no longer require them to be dropped, and yet produce the expected
output on the receiver side.
As far as I understand Opus (specifically section 4.1, 4.3.0 and 4.3.3), the
0s are simply unused bits and are only there because we running Opus in constant
bitrate mode. So, flipping one of them should have no effect on the resulting
audio.

This is a backport of #4720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packet loss caused by #4227
6 participants