Please remove protobuf pom from gwt-servlet

[YSavanier]

Hello

Since there is vulnerability CVE-2021-22569 (see #9659 & #9749) on protobuf 2.5.0 which is detected into gwt-servlet by sonarqube while this is a false positive and no protobuf package is present into the jar (only internal classes), could you please remove the whole folder META-INF\maven\com.google.protobuf please ?

I can't see why there is any need to keep this and it's removal would solve this problem instantly.

Thank you very much.

B.R.

[niloc132]

Thanks for the report - yes, we can look into tweaking the packaging scripts to keep this out. Is that something you'd be interested in looking into fixing for the next release?

It looks like the gwt-servlet.jar packaged in the distribution zip contains both the maven marker and the classes, though the classes are "rebased", with a different package than usual - this is produced by the servlet/build.xml file.

It also appears that the gwt-servlet.jar deployed to maven still includes those classes. It isn't clear at a glance what gwt-servlet needs with those classes - I know that protobuf is necessary for legacy dev mode, but I don't presently see what purpose these serve for a production server.

[tbroyer]

IIRC protobuf is also used for source maps, possibly also the streaminghtmlparser.

[YSavanier]

Hi, thank you for your answer,

Actually the fix is quite simple, in fact this protobuf meta-inf folder content is provided by the unzipping of protobuf-java-rebased-2.5.0.jar located in the tools lib folder at https://github.com/gwtproject/tools/tree/master/lib/protobuf/protobuf-2.5.0.

But while in the readme it is clearly stated at step 3 :

3) Remove the META-INF directory from protobuf-java-rebased-2.5.0.jar. You can do this
   by expanding the jar, deleting the META-INF directory, and rebuilding the jar with the
   -M (no MANIFEST) switch.

This step has clearly not been applied to the protobuf-java-rebased-2.5.0.jar file which still contains this META-INF folder.

So to me, the solution appear just to update a new version of this protobuf-java-rebased-2.5.0.jar with the META-INF folder removed at https://github.com/gwtproject/tools/blob/master/lib/protobuf/protobuf-2.5.0/protobuf-java-rebased-2.5.0.jar

Do you want me to send you the jar file stripped of its META-INF folder ?

But while we are into that pom thing, could it be possible to include the correct gwt artifact pom.xml in the jar files like it was the case until gwt 2.5.2 (so our server dependencies security detection software can spot them correctly ^^) ? Do you want me to open an new issue for this matter ?

Thank you very much

[zbynek]

@YSavanier can you please check if #9785 helps with this?

[YSavanier]

Hi

As far as I can see #9785 is clearly the implementation of the missing step 3 stated in https://github.com/gwtproject/tools/blob/master/lib/protobuf/protobuf-2.5.0/README so yeah, it would clearly solve the reported issue.

Now we just need to solve the missing manifest & pom.xml for gwt-servlet so it can be analyzed by dependencies tools and we are good to go :)

[niloc132]

I just revisited the PR for this and made a small correction. The goal is not to re-upload the binary jar (adding binary files to git isn't ideal, and replacing the jar just to change a file is worse), but instead to modify how the gwt-servlet jar is produced to exclude these. If you use maven, please try adding https://repo.vertispan.com/gwt-snapshot/ as a maven repository to your project and specify version 2.11.0-fix-9778-SNAPSHOT. #9790 (comment) shows that the files are no longer included.

Note that no version of GWT has included pom.xml inside the jar, but a simple META-INF/MANIFEST.MF is present. This is due to building with ant and deploying with maven (building with gradle would have a better manifest, but still would be missing the pom.xml, so this shouldn't be too unusual in the maven world).

[ilatypov]

This request and #9778 aim to suppress the detection of a component. The component was detected correctly. Risk identification is currently difficult to automate. An alternative would make the usage promises mandatory and verifiable. This is not implemented by any language or build system yet.

[niloc132]

@ilatypov please see #9659 (comment) - these classes are in their own rebased package to ensure that no user of protobuf would ever pick these up by mistake, and the code that uses the jar only does so when reading deployed files compiled to be served in the app (and even then, only if you use that particular sourcemap/safehtml code in your own server). The classes will not be used for reading network traffic, untrusted or otherwise.

If this is still unacceptable, we would welcome a contribution of effort/time/money to overhaul these few systems to use a newer version or a different mechanism, but the goal of suppressing the incorrect warning does satisfy the requirement of ensuring that users are alerted to insecure components, and avoiding false positives.

[ilatypov]

The usage details in the above comments alone are sufficient for current human-controlled risk tracking systems (assuming the details are presented with the full circumspection). Bypassing component detection now can recoil in the future if another vulnerability that is relevant to the usage in this project is found in the suppressed component.

I am not saying that the commit changed too little. It changed too much. There is no need in a change, with the current inadequate precision of automatic risk detection and with the current lack in enforcing selective use of components.

[YSavanier]

Hello

Tell me if I'm wrong but if the classes are only used for clientside sourcemapping at compile time as stated in #9659 then thoses classes are never called and can't be called server-side without using reflection, thus no proven CVE exploit can be affected to them, so leaving a protobuf pom.xml will undoubtly flag false positive exploits.

In the other hand, the lack of a gwt-servlet pom.xml will prevent detection systems to flag existing gwt exploits correctly, so if there is a security lack in my opinion it's there, not in the removal of a file firing false positive CVE in every existing GWT project.

Regards.

[tbroyer]

Hello

Tell me if I'm wrong but if the classes are only used for clientside sourcemapping at compile time as stated in #9659 then thoses classes are never called and can't be called server-side without using reflection, thus no proven CVE exploit can be affected to them, so leaving a protobuf pom.xml will undoubtly flag false positive exploits.

That's not entirely exact.

Sourcemaps and streamhtmlparser libraries use protobuf types (they have classes that extend protobuf Message and Builder). Those two libraries are internal dependencies of GWT (not directly exposed to consumers), and all have been repackaged under com.google.gwt.thirdparty and com.google.gwt.dev.protobuf so they can't conflict with other libraries. No code in GWT itself will expose those protobuf objects to client code (except possibly through exceptions, but I'm not even sure). Those two libraries are only called on data that's known at build-time, or only available server-side under the control of the application (parsing getServletContext().getResource(…) files). Looking closely at the code, there's one exception, under a very specific condition: if user code calls SafeHtmlBuilder#appendHtmlConstant and previously called SafeHtmlHostedModeUtils.setForceCheckCompleteHtml(true), or SafeHtmlHostedModeUtils.setForceCheckCompleteHtmlFromProperty (with a com.google.gwt.safehtml.ForceCheckCompleteHtml system property evaluating to true). And even in this situation, this would call streamhtmlparser server-side, but that still wouldn't use protobuf serialization.

So, while there is vulnerable code available in the classloader, no code path will ever actually exercise it, unless there's another vulnerability in the application leading to an RCE of sort, or if the application code directly calls it.

With all those precisions, I do agree with your comment that leaving the protobuf POM inside the JAR would only trigger false positives.
An application directly calling this code would be wrong in every possible way.

Could we go the last mile and effectively remove the vulnerable code altogether? Possibly, but it would cost way too much effort for some improbable edge cases. In the mean time, "hiding" protobuf is the best we can do that wouldn't trigger false positives everywhere.

For completeness: GWT directly uses protobuf serialization in a very specific case, at development time: when launching DevMode with the -remoteUI argument. This is meant to integrate DevMode into IDEs, and is used in the Eclipse plugin so that the DevMode UI is actually an Eclipse pane, talking to the actual DevMode using protobuf. IntelliJ IDEA might be using this too.

[niloc132]

With that said, the PR is on hold until the test build produced from it has been validated and found to not produce this false positive. Once that has been confirmed, we can merge it.

[ilatypov]

Hiding the use from the scanners makes both the development time use of components and the true positive detection by scanners difficult. Separate the concerns and let the plaintiffs work on their own risk re-evaluations, with the help of comments in this thread. (The manual re-evaluations are a sad reality for many other projects. Proper general automation of useage controls will take someone's strong inspiration to resolve). This issue's omitting the original concern and jumping to a conclusion in the request itself does not look necessary.

[YSavanier]

Hiding the use from the scanners makes both the development time use of components and the true positive detection by scanners difficult. Separate the concerns and let the plaintiffs work on their own risk re-evaluations, with the help of comments in this thread. (The manual re-evaluations are a sad reality for many other projects. Proper general automation of useage controls will take someone's strong inspiration to resolve). This issue's omitting the original concern and jumping to a conclusion in the request itself does not look necessary.

Hi.

If there is really a need to point the protobuf vulnerabilities, then the correct way to do it is stil to remove the protobuf pom.xml and correctly provide a gwt-servlet pom.xml AND THEN file dedicated CVEs on it once having checked if the existing protobuf CVEs really apply on those internal classes with a reevaluated scoring relative to their real exploitability.

To me it is plainly wrong to obfuscate the use of gwt (which may host CVE of its own) by not providing its pom.xml and to provide one of another lib whose classes aren't provided to be used as the genuine one.

Regards

[Lonzak]

I didn't follow the previous discussion completely and would like to ask about the status of this issue. Scanners like grype, trivy, docker scout and others report that protobuf (2.5.0) is found. At first I thought that this is a False Positive since the dependency was nowhere to be found. After some digging it turns out it is part of the gwt-servlet dependency 😮

Protobuf in version 2.5.0 contains several CVEs and that's what makes the security guys worry ;-)
The discussion about this started quite some time ago however due to some unknown reason it wasn't changed in the lately released versions 2.10.1 and 2.11.0.

From what I understand there are two things:

1. The protobuf pom.xml is distributed along side gwt-servlet.jar (META-INF\maven\com.google.protobuf\protobuf-java\pom.xml)
2. The binaries of the complete protobuf library 2.5.0 (249 class files) is also distributed along side gwt-servlet.jar. Only the package was changed to com.google.gwt.dev.protobuf instead of com.google.protobuf to avoid accidental usage.

From what I read is the classes are needed and it would be quite an effort to change that.

With that said, the PR is on hold until the test build produced from it has been validated and found to not produce this false positive. Once that has been confirmed, we can merge it.

So that didn't happen? Can the community support with that?

Currently there are 6 CVEs reported for protobuf 2.5.0
CVE-2022-3171 (GHSA-h4h5-3hr4-j3g2)
CVE-2022-3509 (GHSA-g5ww-5jh7-63cx)
CVE-2022-3510 (GHSA-4gg5-vx3j-xwc7)
CVE-2015-5237 (GHSA-jwvw-v7c5-m82h)
CVE-2021-22569 (GHSA-wrvw-hg22-4m67)
CVE-2021-22570 (GHSA-77rm-9x9h-xj3g)

[niloc132]

The conversation and the linked PR are the best place to get the info, but here's the short recap:

• This version of protobuf is not used for network traffic by gwt-servlet.jar (and is not present in requestfactory-server.jar), but only to decode local files. Likewise, the classes have been re-namespaced, so they can't accidentally be used by a server's own other generated Message types. As such, this is a false positive.
• The linked PR, Fixes #9778 #9785, is just over a year old - we published jars when we thought the fix was ready to ask anyone interested in landing this to validate that it was fixed. Without being confident of that, there is no point in merging the fix.
• In that time, no one has validated the fix, so it was never merged, and didn't make the 2.11.0 release (and 2.10.1 only had a real security issue backported to it).
• Updating our sourcemap support would require two different version of protobuf, since legacy dev mode (when enabled) has clients built with an ancient protobuf version.
• Since then we've been working on Enable embedding sources inside the sourcemaps json as sourcesContent #9936 to remove the dependency entirely (from gwt-servlet.jar at least - gwt-dev.jar will still use it for legacy dev mode, if enabled). We attempted to upstream the fix to closure-compiler, but they weren't interested, so we've patched the dependency ourselves (see the patch files in update sourcemaps deps to support embedding sources in source maps tools#31). This fix will not be able to be backported as-is, since closure-compiler now requires Java 11, but GWT 2.11.x may be run on Java 8.

If you'd like to test the old patch, the jars are still deployed where I put them a year ago (maven repo https://repo.vertispan.com/gwt-snapshot/ with version 2.11.0-fix-9778-SNAPSHOT). If you'd like to discuss backporting some/all of that fix, the work isn't too difficult, but will need to be backported on closure-compiler to before they dropped Java 8 support, and #9936 updated (or a new patch made) - feel free to take this up yourself, or to discuss on/off list how to arrange to get this work done.

[Lonzak]

Sure no problem - I'll let the scanners run to verify the result.

If you use maven, please try adding https://repo.vertispan.com/gwt-snapshot/ as a maven repository to your project and specify version 2.11.0-fix-9778-SNAPSHOT. #9790 (comment) shows that the files are no longer included.

I downloaded your file and checked it just by unzipping it - however the protobuf pom is still inside.
gwt-servlet-2.11.0-fix-9778-20221116.163816-1.jar\META-INF\maven\com.google.protobuf\protobuf-java\pom.xml

Wasn't that the goal to have this removed?

[niloc132]

Sorry, as I said, that was a short recap - the summary is not the full picture.

You missed two key parts:

• The fix was published about a year ago, you appear to have accidentally downloaded much older artifacts (see the date, 2022-11-16, you want 2023-03-26 in the exact same directory). The PR discusses this through its history - I added a new patch, and then uploaded a new set of artifacts.
• Opening the zip and looking at its contents by hand may or may not be sufficient to validate that this issue has been fixed. In order to validate it, someone needs to actually depend on this, and confirm that their vulnerability scanner is satisfied - after all, this isn't about a human noticing a file being there or not, but about whether or not it sets off the validation tooling.

Once that fix has been confirmed, we can discuss a 2.11.1 bugfix release, but as noted, we're prepared to abandon that patch and move forward with instead patching closure-compiler's sourcemap tooling in 2.12.

[Lonzak]

the fix was published about a year ago, you appear to have accidentally downloaded much older artifacts (see the date, 2022-11-16, you want 2023-03-26 in the exact same directory). The PR discusses this through its history - I added a new patch, and then uploaded a new set of artifacts.

Ah ok I missed the 2nd file - I downloaded the 2nd file and there it is correct.

Opening the zip and looking at its contents by hand may or may not be sufficient to validate that this issue has been fixed. In order to validate it, someone needs to actually depend on this, and confirm that their vulnerability scanner is satisfied - after all, this isn't about a human noticing a file being there or not, but about whether or not it sets off the validation tooling.

Opening in a ZIP file was only the first step. As a second step I will integrate it and start a scan to see whether it has been fixed. But the 2nd step wouldn't make sense if the file still contained the pom... ->So now I can continue :-)

[Lonzak]

Ok I did add the file and did a docker scout scan and voilà no protobuf 2.5.0

Note: The other protobuf is from wildfly - this can be ignored (different thing)

Conclusion: The fix works and removing the pom successfully prevents this false-positive

So would a gwt-servlet 2.11.1 be possible?

[niloc132]

Given that this is merely a false positive, and that 2.12 will actually fix this by removing the dependency entirely from gwt-servlet, I'm leaning towards not cutting a release just for this since that takes a fair amount of volunteer time that we'd rather spend on shipping concrete improvements.

That said, if there are other patches we'd like to see go out for 2.11.1 (or earlier) and/or if a team wants to sponsor this release, we can consider it, even for something superficial like this.

[Lonzak]

Ok then it depends on the timeframe of the 2.12. release for me. If I extrapolate from the the last release cycle times we can expect version 2.12.0 earliest in Q3 2025. That is too far away I fear...
It might me superficial for you, however each security alert (false positive or not) causes a lot of follow-up work. Analysis, Discussions, Re-evaluations and so on. I mean, it would be pretty bad if you could simply declare all unpleasant warnings as false positives - thus the hurdle to do that is really high for us.
But I can understand your point of view and I am already knee-deep into the topic so I'll probably create our own fork for now if there is no 2.11.1...

Still thank you for your time!

[niloc132]

The expectation is that GWT 2.12 is going to mostly be centered around Java 17 language support - all but the final point has a working draft already. I intend/anticipate that the release will be in 2024, and likely Q3.

I certainly understand your perspective that getting out a clearly-clean build will have advantages for consumers of GWT, and the risks that this poses (your valuable time) and appears to pose (the security if your application). I likewise hope you can appreciate that the majority of the effort of most of the contributors here (myself included) is volunteer work.

[Lonzak]

Ich hoffe auch, dass Sie verstehen können, dass der Großteil der Bemühungen der meisten Mitwirkenden hier (mich eingeschlossen) ehrenamtliche Arbeit ist.

Most definitely! Since I am also an active volunteer in several open source projects I do appreciate and understand that very well.

PS: The only thing I don't quite understand why nobody tested the fix when it was implemented... But it didn't happen, so there's no point in pondering it...

In that time, no one has validated the fix, so it was never merged, and didn't make the 2.11.0 release (and 2.10.1 only had a real security issue backported to it).