1900353 - lgglue.c conditional name is misleading, needs better comments

Status: NEW

(NSS :: Libraries, task)

Description

[Eric Huber]

Steps to reproduce:

I was reading the source code for the primary password hashing after finding some old articles discussing the use of a single iteration in older versions of Firefox. It looks like the default is set to 10,000 iterations now, but there's a code path in what appears to be a legacy DB handler that still defaults to 1 even if flags are set that should disallow that.

Actual results:

Refer to the code block here:
https://searchfox.org/mozilla-central/rev/dc08a7321a22918d5a26b7641f9b10cd2a09d98e/security/nss/lib/softoken/lgglue.c#198-206

When sftk_isLegacyIterationCountAllowed() returns false, the else branch is chosen and so "iterationCount = 1" happens anyway. If I understood correctly, 1 is the legacy iteration count. Should it really end up getting set to 1 here if the legacy iteration count is not allowed?

Expected results:

In contrast, you can see this other code block:
https://searchfox.org/mozilla-central/rev/dc08a7321a22918d5a26b7641f9b10cd2a09d98e/security/nss/lib/softoken/sftkpwd.c#1390-1394

I notice that in part, it checks "!sftk_isLegacyIterationCountAllowed()" instead before setting the count to 1. (Note the "!" operator.)

Comment 1

[Daniel Veditz [:dveditz]]

Kai: looks like you landed this change in bug 1562671. Is there a missing ! here?
https://searchfox.org/nss/rev/ab1cd707af311bfaeece2688adbf0e5f7fafab3d/lib/softoken/lgglue.c#198-206

Comment 2

[Kai Engert [:KaiE:]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Is there a missing ! here?

No, the code is correct, in my understanding.

Eric pointed us to 2 places. In both places, if !sftk_isLegacyIterationCountAllowed() is true, we set the count to 1.

Eric, I think you misunderstood the meaning of that.
The misunderstanding might have been caused by an ambiguous naming of the function.

The word "legacy" doesn't refer to "count 1".
Rather, the word "legacy" refers to an older storage file format (DBM).

If we are using the legacy storage format, then the default is to use iteration count 1.

Using an iteration count > 1 for the legacy DBM storage requires that someone "allows" it.
The only way to allow it is by setting an environment variable.

So the function name sftk_isLegacyIterationCountAllowed
should be interpreted as
"are we allowed to use a larger iteration count with legacy storage?"

Comment 3

[Kai Engert [:KaiE:]]

We could add a comment to make that clearer.

Comment 4

[Eric Huber]

In both places, if !sftk_isLegacyIterationCountAllowed() is true, we set the count to 1.

I see, thanks for clarification. That does seem consistent between the two blocks. As you point out, the intended meaning of the helper function is the opposite of what I thought. Maybe a comment or a different function name would help others like me to understand it, but if there's no issue here, please feel free to close the ticket.