Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Sep 8, 2021

Backport of #58736 to release/6.0

Customer Impact

Customers using the Rfc2898DeriveBytes.Pbkdf2 on Windows 8, Windows 8.1, Windows Server 2012 and Windows Server 2012R2 will get suboptimal performance due to mis-detection of available OS features. The performance of this API is important in web scenarios for hashing passwords.

The code was originally looking for version "8.0" of Windows, however Windows 8.0 reports itself as Windows version 6.2.

Testing

The improved functionality had existed tests and was being exercised in Windows 10. This is just making that functionality available to Windows 8.

Risk

Low. The functionality was already being exercised in Windows 10 and the code change is a small adjustment to a Windows versions detection.

teo-tsirpanis and others added 2 commits September 8, 2021 18:51
Windows 8's actual version is 6.2, not 8. The test would succeed only on Windows 10+.
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
@ghost ghost added the area-System.Security label Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #58736 to release/6.0

/cc @jeffhandley @teo-tsirpanis

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Security

Milestone: -

@jeffhandley jeffhandley changed the title [release/6.0] Fix Windows 8 detection. [release/6.0] Fix Windows 8 detection in Pbkdf2 Sep 8, 2021
@vcsjones

This comment has been minimized.

@danmoseley
Copy link
Member

Question: we have PR validation on Windows 8. Why did it not fail - do we have a test hole?

Approved. New API in 6.0 that cannot otherwise be used by significant number of customers. Very low risk change. Customer (well, community) reported.

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Sep 8, 2021
@jeffhandley
Copy link
Member

Question: we have PR validation on Windows 8. Why did it not fail - do we have a test hole?

It functionally works with the older Windows API, it just wasn't gaining the performance boost from the new API.

@vcsjones
Copy link
Member

vcsjones commented Sep 8, 2021

Why did it not fail - do we have a test hole?

I don't think so. There are basically two APIs for doing PBKDF2, and both of them work. One is just much faster and only available on Windows 8+. In pseudo code, it looked like this:

if (WindowsVersion >= 8.0) {
    DoItTheFastWay();
} else {
    DoItTheSlowWay(); // Intended for Windows 7 only
}

But Windows 8.0's actual version is 6.2 as the OS reports it. So it was falling in to the else, but everything worked.

Windows 10 actually reports its version is 10, and since 10 >= 8, it was hitting the fast path as expected, so we know the DoItTheFastWay() works and is being tested.

@danmoseley
Copy link
Member

Ah - missed that. Thanks for the info.

@danmoseley danmoseley merged commit 06b23ba into release/6.0 Sep 9, 2021
@danmoseley danmoseley deleted the backport/pr-58736-to-release/6.0 branch September 9, 2021 15:00
@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Security Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants