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.

Sign up for GitHub

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

Jump to bottom

Consider making some of the private headers public #721

Open
sorenstoutner opened this issue Jul 26, 2024 · 14 comments
Open

Consider making some of the private headers public #721

sorenstoutner opened this issue Jul 26, 2024 · 14 comments

Comments

@sorenstoutner
Copy link

I work on packaging Chromium (and Qt WebEngine) in Debian. One of my projects is to get to the stage where Chromium can be built using a system copy of libsrtp instead of the version that is included in the Chromium codebase. Doing so has positive security implications because an update to the system copy of libsrtp can fix a security problem in all instances of the Chromium and derivitive packages in Debian.

The blocker for doing so is that Chromium utilizes some of the private headers in the libsrtp package. This is discussed in the following Chromium issue:

https://issues.chromium.org/issues/40272799

Unfortunately, this issue is currently marked private. I have made a request for the visibility to be opened up, but in the meantime I don't think anyone would mind me quoting this section:

The include from webrtc/pc/srtp_session.c seems harder to remove. It's used in GetRtpAuthParams and GetSendStreamPacketIndex with calls originating from sending packets with external auth. I haven't followed this codepath, but I believe the external auth is used for things like the abs-send-time header extension where chrome needs to update the header extensions close to the socket and then reapply the authentication.

The purpose of this feature request is to see how willing the libsrtp developers would be to make the necessary parts of this private header public. Additionally, to understand if there are any negative implications to doing so.

For a little bit of background, see the following bug reports:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=866784

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1038240
@paulej
Copy link
Contributor

paulej commented Jul 26, 2024

One of the concerns with making private parts public is that there is no guarantee that the private parts will not change. That said, with the latest version of srtp (still under development), some of the public APIs have also changed, though those will be documented.

IMO, it would be wiser to provide APIs to allow access to read-only information, as opposed to exposing more of the internals.

@sorenstoutner
Copy link
Author

Both of those are good points. I have requested that one of the Google developers comment here explaining exactly what they need. Perhaps it is already handled by your new public APIs.

@paulej
Copy link
Contributor

paulej commented Jul 27, 2024 •
edited
Loading

Given the code I saw in the links you shared, it doesn't appear that level of information is exposed in the new APIs. It might be undesirable to publish such APIs. I suggest we consider it once we understand exactly what information is needed. If you can collect the list of data that's needed, then everyone can evaluate the best approach to take.

@pabuhler
Copy link
Member

@sorenstoutner, if they are indeed updating parts of the authenticated packet and need to "re authenticate" then I would also suggest that they look at restructuring the code so it does not protect the packet until the last possible moment. How would this re auth even work with AEAD / GCM ciphers.
But like Paul said if we understand the requirements then we can look at changing the API to accommodate.

@sorenstoutner
Copy link
Author

I agree with you. I hope that someone from Google will engage in this conversation. I will politely remind them again in about a week. Otherwise, there isn't much we can do without their input.

@fippo
Copy link
Contributor

fippo commented Aug 13, 2024

tagging @alvestrand (and I wonder whether the "external auth" stuff actually stamps abs-send-time in the network process)

@pabuhler
Copy link
Member

Will close this issue soon if there is no follow up.

@fippo
Copy link
Contributor

fippo commented Dec 21, 2024

Oh this is basically the same as #738 -- I have a libWebRTC change to remove that usage but it might take a while to land. No libsrtp action needed I think

@fippo
Copy link
Contributor

fippo commented Jan 28, 2025

Said fix landed in https://chromiumdash.appspot.com/commit/9ff254eaf2cd799a604178ada2a89500794e13a0 in Chromium 134.
Does that resolve the packaging headache for you @sorenstoutner?

@sorenstoutner
Copy link
Author

Thanks for the heads up. I will test it next week and verify if it resolves the issue completely in the Debian packaging.

@sorenstoutner
Copy link
Author

This looks like good progress, but it doesn't fully address the problem.

This file still contains an import directive for the private header:

https://webrtc.googlesource.com/src.git/+/9ff254eaf2cd799a604178ada2a89500794e13a0/pc/srtp_session.cc#38

In order to build against a system version of libsrtp, the Google code shouldn't include any references to the private header. My initial analysis found problematic import directives in the following files:

third_party/webrtc/tools_webrtc/iwyu/mappings.imp
third_party/webrtc/pc/external_hmac.h
third_party/webrtc/pc/srtp_session.cc

Some progress has been made. As of Chromium 133, third_party/webrtc/pc/external_hmac.h has been resolved. However, third_party/webrtc/tools_webrtc/iwyu/mappings.imp still contains the problematic import.

https://sources.debian.org/src/chromium/133.0.6943.53-1/third_party/webrtc/tools_webrtc/iwyu/mappings.imp/#L22

For the sake of completeness, I should note that my original analysis show imports of srtp_priv.h in all of the following files. The libsrtp files themselves are fine, as the Debian build would remove them and use the system copy of libsrtp0-dev. The fuzzer is not necessary for the Debian packaging, so it can go or stay as suits everyone best.

third_party/libsrtp/test/srtp_driver.c:#include "srtp_priv.h"
third_party/libsrtp/test/dtls_srtp_driver.c:#include "srtp_priv.h"
third_party/libsrtp/srtp/ekt.c:#include "srtp_priv.h"
third_party/libsrtp/srtp/srtp.c:#include "srtp_priv.h"
third_party/libsrtp/include/srtp_priv.h: * srtp_priv.h
third_party/libsrtp/include/ekt.h://#include "srtp_priv.h"
third_party/libsrtp/BUILD.gn:    "include/srtp_priv.h",
third_party/webrtc/tools_webrtc/iwyu/mappings.imp:{ include: ['"rdbx.h"', "private", '"third_party/libsrtp/include/srtp_priv.h"', "public"] },
third_party/webrtc/tools_webrtc/iwyu/mappings.imp:{ include: ['"auth.h"', "private", '"third_party/libsrtp/include/srtp_priv.h"', "public"] },
third_party/webrtc/pc/external_hmac.h:#include "third_party/libsrtp/include/srtp_priv.h"
third_party/webrtc/pc/srtp_session.cc:#include "third_party/libsrtp/include/srtp_priv.h"
testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:#include "third_party/libsrtp/include/srtp_priv.h"

@fippo
Copy link
Contributor

fippo commented Feb 18, 2025

IWYU should not matter but I can remove rdbx.h from there, thank you!

The remaining reason for the include from webrtc/pc/srtp_session.cc is mostly the "external hmac" feature (I think but the compiler will tell). The origin of that is unclear (added here) but I could not find any public bugs. Would putting more of that under ENABLE_EXTERNAL_AUTH ifdef guards help?

@alvestrand you are our only hope now!

@alvestrand
Copy link

@havardk and @jonasoreland may have opinions.

@alvestrand
Copy link

The critical call seems to be in GetSendStreamPacketIndex, which references a srtp_hdr_t* - this is called from ProtectRtp.
I read the code (quickly) as trying to get the rollover counter. There may be "official" ways to do that.

The other one seems avoidable through code deletion - only called from tests as far as I can tell.

egege pushed a commit to egege/webrtc-src that referenced this issue Feb 21, 2025
@fippo
srtp: stop using private libsrtp function to determine packet index
9ff254e 
instead use the standard API to get the rollover counter and
determine the extended sequence number which is the basis for the packet index.

See cisco/libsrtp#738 and
cisco/libsrtp#721

BUG=webrtc:357776213

Change-Id: I90c5a4a538f56132158aa48db8700187fcdb47d2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371960
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Cr-Commit-Position: refs/heads/main@{#43802}
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.webrtc that referenced this issue Feb 21, 2025
@fippo
srtp: use WebRTC to parse the SSRC instead of a libsrtp private function
3d544b2 
See cisco/libsrtp#721

Exercised by SrtpSessionTest.ProtectGetPacketIndex

BUG=webrtc:361372443

Change-Id: I46c1eedc16728cbe5f39aef8760f6789bc1aad9b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/377801
Reviewed-by: Harald Alvestrand <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Cr-Commit-Position: refs/heads/main@{#43957}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 4, 2025
@jan-ivar
Bug 1948685 - Vendor libwebrtc from 9ff254eaf2
d8fc8df 
Upstream commit: https://webrtc.googlesource.com/src/+/9ff254eaf2cd799a604178ada2a89500794e13a0
    srtp: stop using private libsrtp function to determine packet index

    instead use the standard API to get the rollover counter and
    determine the extended sequence number which is the basis for the packet index.

    See cisco/libsrtp#738 and
    cisco/libsrtp#721

    BUG=webrtc:357776213

    Change-Id: I90c5a4a538f56132158aa48db8700187fcdb47d2
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371960
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#43802}
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Mar 5, 2025
@jan-ivar
Bug 1948685 - Vendor libwebrtc from 9ff254eaf2
d35329a 
Upstream commit: https://webrtc.googlesource.com/src/+/9ff254eaf2cd799a604178ada2a89500794e13a0
    srtp: stop using private libsrtp function to determine packet index

    instead use the standard API to get the rollover counter and
    determine the extended sequence number which is the basis for the packet index.

    See cisco/libsrtp#738 and
    cisco/libsrtp#721

    BUG=webrtc:357776213

    Change-Id: I90c5a4a538f56132158aa48db8700187fcdb47d2
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371960
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#43802}
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 5, 2025
@jan-ivar
Bug 1948685 - Vendor libwebrtc from 9ff254eaf2
c4575f0 
Upstream commit: https://webrtc.googlesource.com/src/+/9ff254eaf2cd799a604178ada2a89500794e13a0
    srtp: stop using private libsrtp function to determine packet index

    instead use the standard API to get the rollover counter and
    determine the extended sequence number which is the basis for the packet index.

    See cisco/libsrtp#738 and
    cisco/libsrtp#721

    BUG=webrtc:357776213

    Change-Id: I90c5a4a538f56132158aa48db8700187fcdb47d2
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371960
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#43802}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fippo @alvestrand @paulej @pabuhler @sorenstoutner