373145 - PM Action Attribute Check Should Be At Top Of Loop

Status: RESOLVED INVALID

(Toolkit :: Password Manager, defect)

Description

[Robert Chapin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

From Bug #360493, there remains a concern that the action realm is checked in the middle of the credential DataEntry loop, instead of at the top of the loop.

Referring of course to nsPasswordManager::FillDocument()

Reproducible: Always

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> From Bug #360493, there remains a concern that the action realm is checked in
> the middle of the credential DataEntry loop, instead of at the top of the loop.

What is the concern? Why is that problematic?

Comment 2

[Justin Dolske [:Dolske]]

This seems like a simple and safe change, and avoids running through the ugly field-finding logic when we would just bail out anyway. I was initially leaning towards taking this change.

However, I don't think it meets the criteria for a branch change. I don't see any obvious way this could lead to a security issue. If there's something exploitable in the field-finding logic, an earlier action URL check wouldn't help... The attacker would just slip pass the check by using the proper action URL.

A patch for trunk wouldn't hurt, but since pwmgr is already being rewritten for trunk it's not an worthwhile use of r=/sr= time.

Comment 3

[Robert Chapin]

For the record, I am very concerned about this bug and I don't think "invalid" is the correct status.  at https://bugzilla.mozilla.org/show_bug.cgi?id=360493#c349 the author of this code acknowledged this problem and did not say the concern was invalid.