[MIR] webrtc-audio-processing

[David Henningsson]

Rationale:
This package contains an echo canceller which has a higher quality than the speex echo canceller. If this package was in main, we could build PulseAudio with webrtc echo canceller support. People in bug 1261666 are requesting this.
Also, Debian already does this.

Security:
There are some hits for "webrtc" here: http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=webrtc - but I verified that they all concern other parts of webrtc, i e, the webrtc-audio-processing source package does not contain the source file(s) referenced in the CVE.

QA:
No open bugs in the Ubuntu bug tracker.
One bug in the Debian bug tracker, which has been left unattended for some months.
There is no debian/watch file.

Dependencies:
It has almost no dependencies. Binary packages depend only on libc6/libgcc1/libstdc++6.

Maintenance:
Should normally be in sync with Debian (is currently not, due to the bug mentioned above). Suggest ubuntu-audio team to get bug reports once this gets into main.

[Puyol (paul9510)]

Hello
I installed this package already "web-audio-processing" but when i use : pactl load-module module-echo-cancel aec_method=webrtc it return : Failure: Module initalization failed!!

[David Henningsson (diwic)]

> Hello I installed this package already "web-audio-processing" but when i use : pactl load-module module-echo-cancel
> aec_method=webrtc it return : Failure: Module initalization failed!!

Yes, this is because pulseaudio builds without webrtc support by default. Just installing the package is not enough. You will need to rebuild pulseaudio with libwebrtc-audio-processing-dev installed (and maybe there needs to be some packaging changes to PA too, not sure).

[Puyol (paul9510)]

sorry for the inconvenience, so i have to modify the source code or what, how to rebuild pulseaudio with libwebrtc-audio-processing-dev?

[David Henningsson (diwic)]

> sorry for the inconvenience, so i have to modify the source code or what,
> how to rebuild pulseaudio with libwebrtc-audio-processing-dev?

Here's one of all links that explains how to rebuild a package:

http://www.cyberciti.biz/faq/rebuilding-ubuntu-debian-linux-binary-package/

Also, before running dpkg-buildpackage, you will also need to modify debian/pulseaudio.install and add this line:

usr/lib/pulse-*/modules/libwebrtc-util.so

After installing the resulting .debs (only those corresponding to a package you already have installed, though), and rebooting your system, you should be able to try the webrtc echo canceller.

[Michael Terry (mterry)]

* Needs a team bug subscriber, for whomever will look after this in Ubuntu

* I'm seeing the following compile warning which seems troubling. Any comment on that?

  CC libagc_la-analog_agc.lo
analog_agc.c: In function 'WebRtcAgc_Init':
analog_agc.c:1651:24: warning: iteration 10u invokes undefined behavior [-Waggressive-loop-optimizations]
         stt->env[0][i] = 0;
                        ^
analog_agc.c:1649:5: note: containing loop
     for (i = 0; i < 20; i++)
     ^

* I will assign to security team for a quick "those other CVEs don't bother us" check

[David Henningsson (diwic)]

> * Needs a team bug subscriber, for whomever will look after this in Ubuntu

I suggest ubuntu-audio team.

> * I'm seeing the following compile warning which seems troubling. Any comment on that?

Looking at the actual code, it makes sense to rewrite as

for (j = 0; j < 2; j++)
    for (i = 0; i < 10; i++)
        stt->env[j][i] = 0;

...or just a memset. I e, it wasn't as scary as it first looked.

[Michael Terry (mterry)]

Yar, it's probably an easy fix. But the warning implies that optimization will make the current code result in undefined behavior? Is there an actual problem with the current code or is it an ignorable warning?

[David Henningsson (diwic)]

 > Is there an actual problem with the current code or is it an ignorable warning?

My gut says it's an ignorable warning, but I wouldn't bet on it - I'd prefer to fix it using the code above. Want me to make a debdiff you can sponsor?

[David Henningsson (diwic)]

And fixed upstream:

https://code.google.com/p/webrtc/source/diff?spec=svn2321&r=2321&format=side&path=/trunk/src/modules/audio_processing/agc/analog_agc.c

[David Henningsson (diwic)]

Here's a debdiff for the compiler warning.

[Seth Arnold (seth-arnold)]

I reviewed webrtc-audio-processing version 0.1-2ubuntu2 as checked into
utopic. This review should not be considered a full security audit but
rather a quick gauge of maintainability.

- This packages WebRTC code, a collection of codec/protocols used for
  realtime communication.
- Build-depends debhelper, dh-autoreconf
- Depends on nothing
- No cryptopgrahy
- Does not itself do networking
- No daemons
- No pre/post inst/rm scripts
- No init scripts
- No dbus services
- No setuid
- No binaries
- No sudo fragments
- No udev rules
- No tests are run at build time
- No cron jobs
- Build logs mostly clean, unlikely to be security-relevant

- No subprocesses are spawned
- Extensive manual memory management; many functions assumed preconditions
  are met, but mostly looked safe
- Most file operations are in debugging ifdefs
- Logging looked safe
- No environment variables used
- No privileged portions of code
- No cryptography
- Does not itself do networking
- No temporary files
- No webkit
- No policykit
- Clean cppcheck, only one message, probably false positive

This code is highly technical signal processing code; it's quite possible
that codec-level flaws could have unintended consequences and we could
not possibly repair the protocol without expert assistance. On the other
hand, it looks above-average for signal processing and cleanly separates
the signal processing from other portions of code.

Here are some notes I collected while reviewing; I hope these help
someone:

WebRtcAecm_CreateCore() 16 or 32 byte aligns some members, but the
AecmCore_t structure appears to only allocate 8 or 16 bytes extra for the
buffers; if gcc is already aligning these elements on 8 byte boundaries
wouldn't that be providing 16 (sufficient) or 24 (insufficient) alignment
options? I think the neon 32-byte alignments might not be properly met.

VerifyAndAllocateFragmentationHeader() uses supplied size parameter for
memory allocations and array subscripting (in other methods), no callers
in-tree for this method? Probably needs input validation of some sort.

Why does EchoControlMobileImpl::SetEchoPath() accept a size_bytes
parameter?

Consider calloc() instead of malloc(sizeof (t) * N) to avoid integer
overflow errors. (I didn't follow all the parameters far enough to
determine if there are any exploitable conditions among these instances,
it seemed unlikely.)

Security team ACK for promoting webrtc-audio-processing to main.

Thanks

[Michael Terry (mterry)]

Thanks, Seth! So the compiler warning fix is already in our packaging. And we're back in sync now too! But this still needs a team bug subscriber...

[David Henningsson (diwic)]

@mterry, I just added the ubuntu-audio team as bug subscriber, just as we have for alsa-driver, pulseaudio, etc. Is this what you're looking for, or is it something else?

[Michael Terry (mterry)]

Yup, perfect!

[Matthias Klose (doko)]

Override component to main
webrtc-audio-processing 0.1-3 in utopic: universe/libs -> main
libwebrtc-audio-processing-0 0.1-3 in utopic amd64: universe/libs/extra/100% -> main
libwebrtc-audio-processing-0 0.1-3 in utopic arm64: universe/libs/extra/100% -> main
libwebrtc-audio-processing-0 0.1-3 in utopic armhf: universe/libs/extra/100% -> main
libwebrtc-audio-processing-0 0.1-3 in utopic i386: universe/libs/extra/100% -> main
libwebrtc-audio-processing-0 0.1-3 in utopic powerpc: universe/libs/extra/100% -> main
libwebrtc-audio-processing-0 0.1-3 in utopic ppc64el: universe/libs/extra/100% -> main
libwebrtc-audio-processing-dev 0.1-3 in utopic amd64: universe/libdevel/extra/100% -> main
libwebrtc-audio-processing-dev 0.1-3 in utopic arm64: universe/libdevel/extra/100% -> main
libwebrtc-audio-processing-dev 0.1-3 in utopic armhf: universe/libdevel/extra/100% -> main
libwebrtc-audio-processing-dev 0.1-3 in utopic i386: universe/libdevel/extra/100% -> main
libwebrtc-audio-processing-dev 0.1-3 in utopic powerpc: universe/libdevel/extra/100% -> main
libwebrtc-audio-processing-dev 0.1-3 in utopic ppc64el: universe/libdevel/extra/100% -> main
13 publications overridden.