htdigestparser fails out early when malformed entries are found

• Component: Tools / Tests
• Status: RESOLVED CONFIGURATION CHANGED

[Lucas Forschler]

htdigestparser is used to authenticate users when logging into build.webkit.org

Previously, we simply bailed out if we ran into a malformed entry. This would result in failed authentication, and no logins.
With this change, we will simply skip malformed entries, returning a list that meets our expectations.

[Lucas Forschler]

Created attachment 305288 [details]
Harden the parse logic for htdigestparser

v1 patch

[Build Bot]

Attachment 305288 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/htdigestparser.py:57:  no newline at end of file  [pep8/W292] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 305288 [details]
Harden the parse logic for htdigestparser

This doesn't feel right to me. Normally, one wants to be super unforgiving when dealing with authentication.

[Lucas Forschler]

Created attachment 305290 [details]
v2 patch for style error.

[Lucas Forschler]

This  problem occurred when we moved to an updated database on the new DB server. Previously, I believe the malformed entries were not exported, or possibly there was some flaw that allowed them to pass through.

As it currently works, we never finish parsing the entries. If you are lucky enough to have your credentials in the list before any malformed entry, then you can log in. 

One could argue the solution would be to clean up the database of any malformed entries... but that would not prevent a new one from being created.

[Lucas Forschler]

(In reply to Lucas Forschler from comment #5)
> This  problem occurred when we moved to an updated database on the new DB
> server. Previously, I believe the malformed entries were not exported, or
> possibly there was some flaw that allowed them to pass through.
> 
> As it currently works, we never finish parsing the entries. If you are lucky
> enough to have your credentials in the list before any malformed entry, then
> you can log in. 
> 
> One could argue the solution would be to clean up the database of any
> malformed entries... but that would not prevent a new one from being created.

Actually, I misspoke. With the in place logic, if we run into a malformed entry, we simply bail out... no one can authenticate, as no entries are returned to validate.

[Alexey Proskuryakov]

One surprisingly common scenario in which it is important to parse configs strictly is to protect against a vulnerability where an attacker has partial control over their content. E.g. they could put arbitrary content there, but only with some prefix.

So I don't think that being forgiving is desirable.

[Carlos Alberto Lopez Perez]

(In reply to Alexey Proskuryakov from comment #7)
> One surprisingly common scenario in which it is important to parse configs
> strictly is to protect against a vulnerability where an attacker has partial
> control over their content. E.g. they could put arbitrary content there, but
> only with some prefix.
> 
> So I don't think that being forgiving is desirable.

That patch doesn't cause the auth to be more forgiving.

What this does is to only try to authenticate if the entry with the digest hash meets a minimum requirements.

I think is worse from a security point of view to go ahead and try to authenticate against a hash that is clearly not a md5 one (>32 chars or non alphanumeric) when you know it should be a md5, than to filter that entry and ignore it.

In any case this authentication system (AFAIK) is only used for triggering new builds (clean builds) in build.webkit.org. Years ago we had this without even any auth at all.

[Lucas Forschler]

I can investigate finding and removing malformed entries... Maybe we can filter them out at export time.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #8)

> I think is worse from a security point of view to go ahead and try to
> authenticate against a hash that is clearly not a md5 one (>32 chars or non
> alphanumeric) when you know it should be a md5, than to filter that entry
> and ignore it.


Forget that.
Current behaviour is to disable auth (no one can use the system) if only one entry is wrong. Which is what is happening.

Can't argue about the security of that: its very secure.
But also is very unusable or un-practical: It causes the system to not work.

No idea what caused those wrong entries on the digest file, but the only options I see here are:

 * Be forgiving with those entries
 * Clean the digest file

[Carlos Alberto Lopez Perez]

(In reply to Lucas Forschler from comment #9)
> I can investigate finding and removing malformed entries... Maybe we can
> filter them out at export time.

Generating a new digest file at export time that only contains entries that match mails defined at ./Tools/Scripts/webkitpy/common/config/contributors.json would be ideal.

[Lucas Forschler]

Ling has fixed the entries in the database. Logins should now work... at least until we run into an issue with a newly malformed entry.