Disable autocomplete for password field. Found this in a vulnerabilit…

[kwoodie]

…y scan from Qualys - 86729

See JENKINS-46970.
See JENKINS-46939.

Proposed changelog entries

• Entry 1: auto complete for password field is not disabled, This causes scanners such as Qualys to flag Jenkins with vulnerabilities. Implementing this fix will help enterprise users avoid violations of internal company policies or require them to file security exceptions.

[daniel-beck]

Per https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion, emphasis mine:

Modern browsers implement integrated password management: when the user enters a username and password for a site, the browser offers to remember it for the user. When the user visits the site again, the browser autofills the login fields with the stored values.

Additionally, the browser enables the user to choose a master password that the browser will use to encrypt stored login details.

Even without a master password, in-browser password management is generally seen as a net gain for security. Since users do not have to remember passwords that the browser stores for them, they are able to choose stronger passwords than they would otherwise.

For this reason, many modern browsers do not support autocomplete="off" for login fields:

• If a site sets autocomplete="off" for a form, and the form includes username and password input fields, then the browser will still offer to remember this login, and if the user agrees, the browser will autofill those fields the next time the user visits the page.
• If a site sets autocomplete="off" for username and password input fields, then the browser will still offer to remember this login, and if the user agrees, the browser will autofill those fields the next time the user visits the page.

This is the behavior in Firefox (since version 38), Google Chrome (since 34), and Internet Explorer (since version 11).

IOW, this is the sort of autocomplete=off attribute abuse that resulted in major browsers having to ignore it altogether.

As I wrote in the issue,

• Unconditionally setting it is inappropriate. If you need it, your security policy sucks.
• Adding an option for a pointless attribute adds additional complexity for no benefit at all.

There's simply no way this can be implemented in a sane way.

[kwoodie]

I would caution you to reevaluate your stance. I too have experienced this issue within an enterprise. The issue is that enterprise users need Jenkins, but Jenkins needs to pass security audits and by not accepting this change it will almost always fail a scan. This does nothing but put a black eye on the use of Jenkins and causes people like myself to struggle with getting DevOps tools such as this into my workplace. I am sorry that not all security teams and or places of work are as flexible as what you would like, but ultimately I cannot control the views of the people I work with or other enterprises.

Again, I would ask that you reconsider your stance. If you do not accept this I will be forced to run a custom build that will be troublesome for me and or others that have the same issue.

I am willing to look at alternatives or possibly a setting that would enable and or disable this option on the login screen. Both of which would be significant more effort.

[oleg-nenashev]

If it is a matter of a single flag, we could add it as a feature flag without modifying the default behavior. I will open it to keep the discussion open, but I will keep the "proposed-for-close" label to reflect @daniel-beck's opinion

[daniel-beck]

We already have too many flags for barely used and badly supported features. I fully expect something like that would be missed on a potential Blue Ocean login screen, resulting in a bug report there.

Making it a supported option whose explanation will need to read "This probably won't work because browsers ignore it" also seems pointless.

Not to mention users who expect that features that exist need to work, requesting us to jump through hoops making the option actually effective.

Again, too much hassle for too little benefit.

[daniel-beck]

As above.

(I'm not "requesting changes"; as I commented on the issue months ago, I don't expect I'd consider anything in core acceptable.)

[Wadeck]

As there is like a hunt between browsers that want to quicken navigation, banking company that want to secure their applications and password managers that want to help the user, the solution of today will not work tomorrow.

I am not against a fix but it seems unrealistic to have something that work everywhere, without spending lots of time everytime a browser / a password manager get updated.

[daniel-beck]

@Wadeck Well, as you can see from the Mozilla page I quoted, this solution won't even work today. It's only intended to shut up tools that consider it a security problem to not do this (even though reality is pretty much the opposite).

[mitchellsimoens]

I can understand @daniel-beck 's stance but I also hope he understands there isn't anything we can do about we being forced to need to turn autocomplete off.

Project I am on uses Qualys and it is flagging the j_password field in the login form as not having autocomplete turned off like this PR is adding. Using Qualys and attempting to fix all the issues it is showing is not optional and issues like this one are not optional to fix but what is optional is to use Jenkins. I'd rather not have to switch to something else but if you are refusing to allow this fix will force the issue of switching. It's simply not optional to comply with Qualys scans.

[mschwartz]

I'm not seeing the benefit of rejecting this PR. You're likely to drive people to look for another solution altogether.

Or at least some sort of command line switch or configuration to fix this for real world production environments.

[michaeltintiuc]

I understand that modern browsers do not care about this attribute, however businesses are using the versions that still do and most likely will continue to do so for years to come. I do not see any drawbacks from implementing this PR

[daniel-beck]

@mitchellsimoens As Qualys customer, maybe you can get them to explain the problem with not having this flag? I quote extensively why it's ineffective to begin with, they should be able to explain why they consider it necessary.

FWIW, PR acceptance into Jenkins is consensus based, which means me disagreeing doesn't kill this PR (which is why it's not closed). It just means it needs a bit more support from other maintainers.

[daniel-beck]

A Qualys employee writes:

One thing we may need to look at more closely is that recent versions of major browsers including Chrome, IE11, and Firefox now ignore autocomplete="off" for inputs of type=password. Therefore it is dubious to report this finding on password inputs. It would still be valid on sensitive non-password inputs such as credit card numbers for example.

That was three years ago. What a joke.

[mitchellsimoens]

They have documentation for their vulnerabilities they scan for and this one is claimed to allow information leakage. For the quote of the Qualys employee, this would mean that anything older than IE11 is still vulnerable to this and the market share is still significant especially in government and enterprises who are always slow to upgrade.

I also do agree that this "vulnerability" is small and shouldn't stop the use of Jenkins but I don't make the decision on what matters to my client and their customers. If it's reported, it has to get fixed and I don't currently have any facility to fix this. So here I am, stuck between a rock and a hard place with no way out and normally OSS is more flexible than with commercial companies and lawyers.

[Wadeck]

Even if I agree with Daniel concerning the utility of this feature in recent environment, I can only agree with @mitchellsimoens concerning old browsers, conservative business environment, etc.

From my PoV, we do not increase the risk of attack by accepting this PR, it's just a matter of maintainability cost that is increased.

If we keep only the attribute addition (and no feature flag or something complicated), I will approve this PR in case someone else from core also approve (to make the +1, -1 becoming +2, -1)

[kwoodie]

I am good with my commit as it stands (attribute only). Can we move forward and approve?

[Wadeck]

@kwoodie I meant by "someone else from core", one of the core contributor of the project, i.e. person with merge permission on the project.

[oleg-nenashev]

As I said somewhere before, I see no big problem with merging this PR if it does not break other instances. I do not see what is the problem in this particular case. Does not work reliably? Of course? Does it break something? Nope.

There are multiple requests about it, and I can also imagine my customers coming with the same requirements. So I am +0 about the merge.

If it's reported, it has to get fixed and I don't currently have any facility to fix this.

If somebody blocks the PR,, there are ways to solve the issue:

1. Rework the PR to make the fix more generic somehow. E.g. create a "passwordInput" or "userPassword" Jelly control, and allow overriding it by plugins
2. make a small fork with just this particular patch. It won't cause much overheads. There is also an option to write a small plugin which replaces the login page by your implementation
3. Contribute to [JENKINS-50447] New design and slim down of login page #3380 by @scherler in order to get the change somehow addressed there. E.g. I requested compatibility with old login pages there, and you could contribute in order to get support of custom login pages there somehow

[oleg-nenashev]

@jenkinsci/code-reviewers more feedback will be REALLY appreciated in this case

[daniel-beck]

Perhaps people who care about this should at least attempt to address this issue with the vendor of the tools that produces these false positive findings…? The upside of that, if successful, would be that there would not be a need for this to be addressed with every individual tool used.

I'd like to discourage others from merging this without indication that a genuine effort to have the scanner fixed was made first. So far, there's no indication this has happened.

[oleg-nenashev]

@kwoodie @mitchellsimoens @mschwartz ^ some "indication that a genuine effort to have the scanner fixed was made first" would help to move forward here.

@jenkinsci/code-reviewers ping again

[mitchellsimoens]

Sorry, we ran a test with gitlab and this PR topic and others that Jenkins and bitbucket were red flagged are no longer an issue so we are migrating to gitlab now.

[daniel-beck]

@mitchellsimoens

so we are migrating to gitlab

In response to this comment I took a look at GitLab and cannot find where it does this differently.

https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/views/devise/shared/_signin_box.html.haml seems to be the general login form, with https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/views/devise/sessions/_new_base.html.haml being the password based login. Neither appears to disable autocomplete.

Running the GitLab CE Docker image also doesn't indicate it does anything about autocomplete, I checked http://localhost:8080/users/sign_in and it does not seem to disable password autocomplete anywhere.

What am I missing?

[schmichri]

Clearly an Upvote.
autocomplete=off should be configurable for the purpose of Securtiy Audits, Qualys etc.
Thanks for the efforts to convince the guys and the PR

[kwoodie]

I would like to see this merged in still and closed out. Within my organization it's a painful effort of asking to be excluded from the Qualys vulnerability finding every time we spin up a new jenkins container/server.

[Wadeck]

Thx for the update using last version of login.jelly

[alecharp]

This seems quick simple in order to comply to web convention. +1 for this fix.

[daniel-beck]

web convention

It's not, just a garbage security tool making everyone's life difficult. See my previous comment.

FWIW GitHub doesn't seem to have it either.

[oleg-nenashev]

@schmichri @kwoodie

autocomplete=off should be configurable for the purpose of Securtiy Audits, Qualys etc.

It would not have been a problem if it was configurable. See #3299 (comment) . On May 30 I proposed several ways to address it: #3299 (comment) , nobody seemed to be interested to implement it.

As said above, I am okay with merging it as if it gets enough votes. But there is no obvious consensus so far. Usually pinging @jenkinsci/code-reviewers and @jenkinsci/core helps to get feedback

[batmat]

I agree with @daniel-beck that, though we might agree on adding maintenance burden into Jenkins to accomodate with outdated tools, I would require that people here requesting this change provide some kind of proof they did reach out to that Qualys company through their customer support or whatever telling them that policy/rule (as they know! given the forum link above) does just not make any sense.

[jvz]

What if we had a sort of "paranoia" mode which can be enabled to add additional security hardening. Such things that would get rolled in there would mostly be to satisfy security teams, but perhaps it could be used for security features that seem superfluous.

[jvz]

I'm working on I've published a plugin-based approach that enables this setting here: https://github.com/jenkinsci/paranoia-plugin

[oleg-nenashev]

I believe this pull request can be closed now. Since there is a plugin by @jvz, we should be moving towards extensibility instead of a straightforward implementation

[batmat]

Executing on the last update from Oleg. I also think now the https://github.com/jenkinsci/paranoia-plugin got created, hosted, and released, this can be closed.

Please feel free to comment and explain what I might be missing. We can reopen this anytime if needed anyway.

Thanks a lot!

[Wadeck]

Please take care that the plugin is released but not ready due to a missing part: #3721 that was merged only this week. So we need to wait for the weekly to include it and then to re-release the plugin with that core dependency.
Otherwise if you are using the plugin, the CSS of the login page will be erased by the plugin.

[batmat]

That is true.
Anyway in the end people would anyway have to wait for a future weekly with the current PR if we wanted to merge it.
And I believe that the result here is the right one: we leveraged and improved (thanks Matt) the existing extension point instead of adding a "feature" (other than an extension point, that is) to Jenkins Core.