Libevent/10542 CVE fixes v4.1.x

[BrendanCunningham]

I'd appreciate feedback on the following in particular:

1. I am not the author of these commits but ompi/v4.1.x already uses the libevent code; should I get the original authors to sign off on these? The contribution guidelines say the "Signed-off-by" must come from the commit author.
2. The commit messages do not reference this issue (ompi/v4.1.x libevent code has CVEs that are fixed in libevent/master. #10542).
3. The commit messages have "fixes #" messages for issues in libevent.

bot:notacherrypick

[bwbarrett]

Explicitly nacking so that we don't merge this without discussion. I do not believe that we should take one-off patches. We should either update libevent to one without the CVE issues or (since we never actually build the code in question) just ignore the problem. But strapping in more custom patches goes against everything we've been trying to do with libevent and Open MPI.

[BrendanCunningham]

Explicitly nacking so that we don't merge this without discussion. I do not believe that we should take one-off patches. We should either update libevent to one without the CVE issues or (since we never actually build the code in question) just ignore the problem. But strapping in more custom patches goes against everything we've been trying to do with libevent and Open MPI.

I am in agreement in that I do not like creating a version of libevent in ompi/v4.1.x and ompi/v4.0.x that did not exist in the libevent repo.

I have not assessed the extent of the changes between the current ompi/v4.1.x libevent copy and the first stable libevent after that contains the fixes.

2 of the 3 CVEs addressed by this PR are in evdns.c. evdns.c does not get compiled when I do ./autogen.pl && ./configure --prefix=<path to sandbox dirl> && make -j && make install.

The third CVE is in evutil.c,evutil_parse_sockaddr_port(). Outside of libevent test code, evutil_parse_sockaddr_port() is only called from libevent/evdns.c. Analysis based on the ompi/v4.1.x code. However, evutil_parse_sockaddr_port() did still get compiled into libopen-pal.so:

../STL-63835/lib/libopen-pal.so.40.30.2:0000000000084230 T opal_libevent2022_evutil_parse_sockaddr_port

[jsquyres]

Is this a libevent configure option to disable the DNS code, perchance?

[BrendanCunningham]

Is this a libevent configure option to disable the DNS code, perchance?

Yes, libevent has a configure option --disable-dns.

opal/mca/event/libevent2022/configure.m4:138:    event_args="--disable-dns --disable-http --disable-rpc --disable-openssl --enable-thread-support"

And this carries over to libevent2022/libevent configure when doing ./autogen.pl && ./configure --prefix=${PREFIX} && make -j && make install and libevent/evdns.o is not built:

./opal/mca/event/libevent2022/libevent/config.log:  $ ./configure --disable-dns --disable-http --disable-rpc --disable-openssl --enable-thread-support --disable-evport...

[jsquyres]

@BrendanCunningham Does --disable-dns effectively address the CVEs?

[awlauria]

Refs: #10583

[jsquyres]

@BrendanCunningham Ping.

[jsquyres]

@BrendanCunningham (and @artemry-nv, from #10583) How did your internal scanner find this issue? Do we know that patching libevent will actually make your scanners happy? Or are the scanners just seeing "libevent 1.0.22" and therefore flagging it?

I ask because if the scanner is just flagging the libevent version, then this PR won't be sufficient.

[jsquyres]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bwbarrett]

@BrendanCunningham after discussing options with Jeff, we're leaning towards accepting this PR if we can confirm that the scanners will be happy with Open MPI after this patch. However, can you update the Signed-off-by lines in the three patches to include a valid email address, that would be helpful. As would be a short note on the patches sourcing their origin would be awesome.

In the case of third party packages like this, its unreasonable to have the author add the signd-off-by, so you doing so is the right thing to do. I would also remove the Fixes lines to avoid GitHub getting confused.

[BrendanCunningham]

@BrendanCunningham (and @artemry-nv, from #10583) How did your internal scanner find this issue? Do we know that patching libevent will actually make your scanners happy? Or are the scanners just seeing "libevent 1.0.22" and therefore flagging it?

I ask because if the scanner is just flagging the libevent version, then this PR won't be sufficient.

I don't have access to that scanner now. I think it must have done something beyond checking for a version string since the fix we used for our build of OMPI for the hits in evdns.c was to remove the file evdns.c.

[BrendanCunningham]

@BrendanCunningham Does --disable-dns effectively address the CVEs?

--disable-dns does prevent evdns.c from being compiled; that takes care of 2 of the 3 CVE hits.

The third CVE hit is in evutil_parse_sockaddr_port(); evutil_parse_sockaddr_port() does still get compiled into libopen-pal.so. However, as far as I can tell, outside of test code, evutil_parse_sockaddr_port() is only called from evdns.c.

[jsquyres]

@BrendanCunningham So there's three proposed solutions:

1. See if --disable-dns was sufficient. I.e., is your scanner smart enough to realize that the bad functions in question are dead code or not? It sounds like the answer is "no" -- and that this solution is not sufficient.
2. Use the patches from this PR. This is the easiest solution, but we don't really like including local patches in 3rd party code.
   • That being said, this PR is the smallest/simplest solution -- so it's the leading contender right now.
   • Do you know for a fact that your patches on this PR make your scanner happy? (we have to verify that they make Nvidia's scanner happy, too) If they make your scanner happy, can you fix up your commits as @bwbarrett requested?
3. If this PR is not sufficient to make your + Nvidia's scanners happy, we'll likely need to upgrade to a newer libevent (e.g., the version that's on main). That's a little bit of work back on the v4.x series (i..e, delete the old component and make a new component), but it's do-able.

Also, I'll ask you the same questions I asked @artemry-nv on #10583:

• Do you know how your scanner suddenly found this ~6-year-old CVE? I.e., why is this surfacing now?
• What's the urgency level of this for you? E.g., do you have a customer complaining about it?

[BrendanCunningham]

@BrendanCunningham So there's three proposed solutions:

1. See if `--disable-dns` was sufficient.  I.e., is your scanner smart enough to realize that the bad functions in question are dead code or not?  It sounds like the answer is "no" -- and that this solution is not sufficient.

We have a newer scanner now; it might be smart enough to realize none of the vulnerable code is actually present or callable.

I'm working with one of our CI devs right now to determine the answer to this. I'll get back to you on this.

2. Use the patches from this PR.  This is the easiest solution, but we don't really like including local patches in 3rd party code.
   
   * That being said, this PR is the smallest/simplest solution -- so it's the leading contender right now.
   * Do you know for a fact that your patches on this PR make your scanner happy? (we have to verify that they make Nvidia's scanner happy, too)  If they make your scanner happy, can you fix up your commits as @bwbarrett requested?

Yes, we will test if the PR fixes the CVE hits according to our scanner. I'll fix up my commits as requested.

3. If this PR is not sufficient to make your + Nvidia's scanners happy, we'll likely need to upgrade to a newer libevent (e.g., the version that's on main).  That's a little bit of work back on the v4.x series (i..e, delete the old component and make a new component), but it's do-able.

Also, I'll ask you the same questions I asked @artemry-nv on #10583:

* Do you know how your scanner suddenly found this ~6-year-old CVE?  I.e., why is this surfacing now?

Our scanner did not suddenly find this; we had found this a few years back when OPA was maintained by Intel and had a report on file.

We only recently realized we should fix this in upstream rather than just carrying our fixes from OMPI release to OMPI release.

* What's the urgency level of this for you?  E.g., do you have a customer complaining about it?

Not urgent; we do not have a customer complaining about it. And as we've established, none of the vulnerable code is actually live, so we can defend this if we had to.

[jsquyres]

Yes, we will test if the PR fixes the CVE hits according to our scanner.

@BrendanCunningham Have you been able to do this yet, perchance?

[BrendanCunningham]

Yes, we will test if the PR fixes the CVE hits according to our scanner.

@BrendanCunningham Have you been able to do this yet, perchance?

Have not, no.

[jsquyres]

@BrendanCunningham Have you been able to do this yet, perchance?

Have not, no.

@BrendanCunningham Do you have an ETA to when you can figure out if this PR actually makes your scanner happy?

[BrendanCunningham]

@BrendanCunningham Have you been able to do this yet, perchance?

Have not, no.

@BrendanCunningham Do you have an ETA to when you can figure out if this PR actually makes your scanner happy?

Should be able to answer this by end of next week.

[BrendanCunningham]

@jsquyres we ran our scanner against both our OpenMPI 4.1.4 build with our CVE fixes and Open MPI 4.1.4 built from the openmpi-4.1.4-1.src.rpm. Our CVE scanner did not report any CVEs in either Open MPI build.

EDIT: After this post, we discussed the issue on the weekly OMPI dev Tuesday Webex, and unfortunately the issue is not as clear cut as it initially appeared; there may be cases where there scanner does still detect the CVE. @BrendanCunningham is still investigating, and will post back here shortly.

[BrendanCunningham]

@jsquyres we ran our scanner against both our OpenMPI 4.1.4 build with our CVE fixes and Open MPI 4.1.4 built from the openmpi-4.1.4-1.src.rpm. Our CVE scanner did not report any CVEs in either Open MPI build.

EDIT: After this post, we discussed the issue on the weekly OMPI dev Tuesday Webex, and unfortunately the issue is not as clear cut as it initially appeared; there may be cases where there scanner does still detect the CVE. @BrendanCunningham is still investigating, and will post back here shortly.

@jsquyres after further research, we believe our latest libevent CVE negatives to be true-negatives; our scanner did not find CVEs in Open MPI 4.1.4 either with or without the libevent CVE fixes.

[jsquyres]

@jsquyres after further research, we believe our latest libevent CVE negatives to be true-negatives; our scanner did not find CVEs in Open MPI 4.1.4 either with or without the libevent CVE fixes.

Cool. I'm going to close this PR without merging, then. Still waiting to hear back from NVIDIA on #10583 -- we can re-open this PR if we need to.

[jsquyres]

After much discussion and back-n-forth, we are re-opening this PR.

Per #10583 (comment), it looks like the Black Duck scanner is going to report this CVE even if we apply these patches. And we know that these patches are unused / very low risk in the Open MPI code base. Also, we tried in #10602 to upgrade the Libevent on this branch, but that also turned into a giant tar pit of sadness.

So it seems like the easiest and most responsible thing to do is just apply these upstream patches (even though we really don't like applying patches to our 3rd-party software), be able to state that we have fixed the CVE (even though they don't really apply), accept that Black Duck will still complain anyway, and move on to other things.

I therefore rebased this PR to the current HEAD of the v4.1.x branch (mainly to get rid of the failed Git Commit Checker CI test that failed against the old GitHub Actions pull_request target), and we'll let CI run

[jsquyres]

There's something weird about why the RTD CI test is getting an error from git cloning/checking out this PR; I can't figure out why that would be happening. Best guess is that it has something to do with how this PR was closed for a long time, and just re-opened recently. 🤷‍♂️

I have pushed the commit from this PR to my own fork and opened #10835; the RTD CI test passed there. 🤷‍♂️

So I'm going to close this PR and switch to the #10835 PR.