sonar-quality-gates plugin security issue

[Kirk Fitzsimons]

Hi,

This is my first post here.

I have been looking at https://plugins.jenkins.io/sonar-quality-gates/ and have contributed to the plugin to make it compatible with newer version of jenkins and java etc. 

The plugin has a security issue opened up against it:

Credentials transmitted in plain text by Sonar Quality Gates Plugin

I would like to see if I can resolve it, is there more information available from the security team as to where the issue was found? or how to replicate it locally. Is it in the code? Is it an issue with dependencies(which have since been updated)

What I have do so far is to install secbugs, and run it, nothing came up around credentials.

I have also looked at other plugins that have fixed the issue, e.g
logstash-plugin

From the commit in the logstash plugin I can see that the main change seems to be utilizing the hudson.util.Secret class instead of the String class.

In had a quick look at the sonar-quality-gates plugin classes that handle passwords and I can see that it does utilize the Secret class





Any information on this would be much appreciated.

Kirk

[Daniel Beck]

On Thu, Aug 8, 2024 at 12:34 PM Kirk Fitzsimons <fitzsim...@gmail.com> wrote:

The plugin has a security issue opened up against it:

Credentials transmitted in plain text by Sonar Quality Gates Plugin

I would like to see if I can resolve it, is there more information available from the security team as to where the issue was found? or how to replicate it locally. Is it in the code?

https://github.com/jenkinsci/sonar-quality-gates-plugin/blob/b87a0987c2b91ef63f5c6dd0f7a6839e341b3584/src/main/java/org/quality/gates/jenkins/plugin/GlobalConfigDataForSonarInstance.java#L81-L83 returns the plain text password/decrypted Secret. That value is then shown (masked because password field) on the UI, but the password will show up in the HTML source code. Jenkins forms generally show the existing encrypted value on the UI.

Fix the getter and setter types, adapting any programmatic callers, and it should work as expected.

[Kirk Fitzsimons]

Thanks for your reply.

When i inspect the html this is what I see, maybe I misunderstood, but I thought if the password was transmitted in plain text I would see the actual password e.g 'kirkpassword', not an encrypted password.
If i have misunderstood and the update the the getters and settings is still needed, what should i expect to see in the html?

Kirk

[Kirk Fitzsimons]

I made an attempt at his, i am not sure if it correct as I have not noticed any difference
https://github.com/kirk-fitz/sonar-quality-gates-plugin/commit/72201ae973f6299e35f530154617dff6e4db52da

[Daniel Beck]

On Thu, Aug 8, 2024 at 3:15 PM Kirk Fitzsimons <fitzsim...@gmail.com> wrote:

When i inspect the html this is what I see, maybe I misunderstood, but I thought if the password was transmitted in plain text I would see the actual password e.g 'kirkpassword', not an encrypted password.
If i have misunderstood and the update the the getters and settings is still needed, what should i expect to see in the html?

Testing plugin version1.3.1, the latest at the time of the advisory, on Jenkins 2.204.3, recent at the time, the password is sent in plain text when loading the global config form. The same should apply to hpi:run at its 2020 state.

Since 2.236, Jenkins always round-trips f:password values in their encrypted form, so the behavior from the 2020 security advisory no longer applies to more recent Jenkins releases. I forgot I added that, sorry for the misleading first response. Switching the types around will still prevent constant re-encryption of a plaintext value, which changes the actual value stored on disk (and probably makes e.g. jobConfigHistory worse).

Looking at the UI, I would like the token to be stored and transmitted as a Secret and masked on the UI, before removing this warning, That's essentially the same issue, and I cannot reconstruct why we didn't pick up on it at the time.

[Kirk Fitzsimons]

I have created the follwing PR which i believe addressed he issue
https://github.com/jenkinsci/sonar-quality-gates-plugin/pull/30
If the PR is approved/merged, what is the process for removing the warning? Can this be updated by the security team?
Or does the maintainer need to create a PR against https://github.com/jenkins-infra/update-center2/#security-warnings

Kirk

[Kirk Fitzsimons]

The following PR https://github.com/jenkinsci/sonar-quality-gates-plugin/pull/30 has been merged, can the security team re-evaluate the security advisory?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/13481880-4c38-4ed3-ba2d-440aca7b578bn%40googlegroups.com.

[Kevin Guerroudj]

It indeed looks like this PR fixed the vulnerability for the password.

However, as previously mentioned by Daniel, before removing this warning, we would also like to see the token stored and transmitted as a Secret and masked in the UI.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAD0tVJGNA3QdVWyM9LqbKyXWYSgNj8t_0OdPuS_z%2Bch-6aPU2g%40mail.gmail.com.

[Kirk Fitzsimons]

I have created another PR to address the issue with the token in the code and the UI
https://github.com/jenkinsci/sonar-quality-gates-plugin/pull/34

I am just waiting on the maintainer to see id it looks okay, more than happy for anyone on security team to have a look at it also

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAKG2iZiguxssL49%3DqYig0uknFwc9FfXackykVdvZMfd5S6T_UQ%40mail.gmail.com.