[API Proposal]: Obsolete Rfc2898DeriveBytes constructors with unsafe defaults

[vcsjones]

Background and motivation

The Rfc2898DeriveBytes type has constructors with default values for iterations and hashAlgorithm set to 1000 and HashAlgorithmName.SHA1, respectively.

These defaults are not suitable. 1000 iterations of PBKDF2 is too low, and using SHA1 is discouraged now. We can’t change the defaults as that would be a breaking change, and any new defaults decided on today would not be suitable as defaults in the future. Suggested values can be covered in documentation or external resources.

We should obsolete the constructors that provide unsafe defaults. Developers that are impacted by the obsoletion can either suppress it, or use the constructors that explicitly accept the iterations and hashAlgorithm.

API Proposal

namespace System.Security.Cryptography {
    public partial class Rfc2898DeriveBytes : DeriveBytes {
        // Uses SHA1 by default
        [Obsolete("The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations);

        // Uses SHA1 by default and 1000 iteration default
        [Obsolete("The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, byte[] salt);

        // Uses SHA1 by default
        [Obsolete("The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations);

        // Uses SHA1 by default and uses 1000 iterations by default
        [Obsolete("The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, int saltSize);

        // Uses SHA1 by default
        [Obsolete("The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations);
    }
}

API Usage

No usage.

Risks

These APIs are likely to have high use.

Alternatives

Implement an analyzer if obsoletion is determined to be too disruptive.

There appears to have been some attempt to move folks toward safer defaults in the past, namely in #21760. I’m unsure exactly what the outcome of that was. The link no longer works and I see no notes in the documentation.

This however was done before obsoleting was something that had a paved path, so perhaps it is worth revisiting as a full obsoletion or analyzer.

[ghost]

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

Issue Details

---

Background and motivation

The Rfc2898DeriveBytes type has constructors with default values for iterations and hashAlgorithm set to 1000 and HashAlgorithmName.SHA1, respectively.

These defaults are not suitable. 1000 iterations of PBKDF2 is too low, and using SHA1 is discouraged now. We can’t change the defaults as that would be a breaking change, and any new defaults decided on today would not be suitable as defaults in the future. Suggested values can be covered in documentation or external resources.

We should obsolete the constructors that provide unsafe defaults. Developers that are impacted by the obsoletion can either suppress it, or use the constructors that explicitly accept the iterations and hashAlgorithm.

API Proposal

namespace System.Security.Cryptography {
    public partial class Rfc2898DeriveBytes : DeriveBytes {
        // Uses SHA1 by default
        [Obsolete("Rfc2898DeriveByte constructors with default hash algorithm or iterations is obsolete and not supported. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations);

        // Uses SHA1 by default and 1000 iteration default
        [Obsolete("Rfc2898DeriveByte constructors with default hash algorithm or iterations is obsolete and not supported. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, byte[] salt);

        // Uses SHA1 by default
        [Obsolete("Rfc2898DeriveByte constructors with default hash algorithm or iterations is obsolete and not supported. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations);

        // Uses SHA1 by default and uses 1000 iterations by default
        [Obsolete("Rfc2898DeriveByte constructors with default hash algorithm or iterations is obsolete and not supported. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, int saltSize);

        // Uses SHA1 by default
        [Obsolete("Rfc2898DeriveByte constructors with default hash algorithm or iterations is obsolete and not supported. Use a constructor that accepts the hash algorithm and the number of iterations.")]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations);
    }
}

API Usage

No usage.

Risks

These APIs are likely to have high use.

Alternatives

Implement an analyzer if obsoletion is determined to be too disruptive.

There appears to have been some attempt to move folks toward safer defaults in the past, namely in #21760. I’m unsure exactly what the outcome of that was. The link no longer works and I see no notes in the documentation.

This however was done before obsoleting was something that had a paved path, so perhaps it is worth revisiting as a full obsoletion or analyzer.

Author:	vcsjones
Assignees:	-
Labels:	api-suggestion, area-System.Security, untriaged
Milestone:	-

[GrabYourPitchforks]

This is likely to be a bit tricky. There are two different sets of customers: people who have existing applications which they want to upgrade while maintaining compat, and people who are writing greenfield code who may be misled into using this bad API. Each audience must take a completely different set of resolution steps.

Reminds me of an older FxCop rule which flagged usage of MD5, SHA1, etc. The goal was to steer people toward using better algorithms, but the end result was that significant numbers of developers ended up suppressing the rule entirely. ("Yes, I know the algorithm is busted. Take it up with the people who wrote the RFC I'm implementing.")

A fixer which automatically rewrites call sites from the old ctors to the new ctors may be useful from the perspective of telling devs more clearly what the underlying behavior is. And that can be done independently of any "insecure algorithm!" analyzer.

Edit: An "insecure algorithm!" analyzer would also run into the sticky situation of us declaring policy on whether HMACSHA1 really is insecure, etc. Last I checked, the underlying algorithm being vulnerable does not automatically imply that an HMAC construct built around that algorithm is also vulnerable. At which point we say "well, maybe not insecure, but perhaps inadvisable," and then the desire to do it drops off considerably.

[vcsjones]

the end result was that significant numbers of developers ended up suppressing the rule entirely.

Yeah, and to be honest I think that is a fine outcome. I assume this would have its own diagnostic code. We can’t actually move them to something better without breaking their application. If they suppress the obsoletion, they will still be using bad defaults. If an analyzer “fixes” it for them, then maybe they won’t stop to consider why they were obsoleted in the first place and are still using bad defaults. Going right for the light bulb and “no more squiggles” is something I actually wanted to dissuade. Though at least that approach makes it obvious that SHA1 / 1000 iterations is being used.

An "insecure algorithm!" analyzer would also run into the sticky situation of us declaring policy on whether HMACSHA1 really is insecure, etc.

Yeah, I was not proposing an “insecure algorithm!” analyzer - really just removing our opinions on defaults, which may be interpreted as “use these and you’re good to go”. These defaults are from 2005. I would not, say, propose throwing a runtime exception if SHA1 or a low number of iterations is used.

Last I checked, the underlying algorithm being vulnerable does not automatically imply that an HMAC construct built around that algorithm is also vulnerable. At which point we say "well, maybe not insecure, but perhaps inadvisable," and then the desire to do it drops off

Indeed, there are no known attacks on HMACMD5, either.

[vcsjones]

Perhaps a better issue title would have been "[API Proposal]: Obsolete Rfc2898DeriveBytes constructors with opinionated defaults" but I will refrain from changing the title. 😄

[GrabYourPitchforks]

I mean, I'm content to obsolete every instance method on the type and to ask people to use the static one-shots. But that's perhaps overkill. :)

[bartonjs]

I think I like the idea of using a fixer to be like "hey, you're using some defaults from 2005. I'm going to put them in your face so you can see what decisions you've made". What would the fixer look for? This obsoletion code, perhaps?

[vcsjones]

I think I like the idea of using a fixer to be like "hey, you're using some defaults from 2005. I'm going to put them in your face so you can see what decisions you've made". What would the fixer look for? This obsoletion code, perhaps?

Am I correctly understanding that an obsolete probably won't fly, so we should re-work this idea in to an analyzer?

[bartonjs]

Am I correctly understanding that an obsolete probably won't fly, so we should re-work this idea in to an analyzer?

I started off thinking that. Then realized that Obsolete is its own analyzer, so we should just use that. But I think the fixer is an important aspect of it. (Fixers just fire off of diagnostic codes, pretty sure that we could write one that looks for SYSLIB00NM and rewrites the call to a longer signature.)

The only drawback that I see is someone cross-compiling NS2.0 and NET7, but they can just turn off the diagnostic code if they don't want to suppress or #if.

[terrajobst]

The message seems to simply that something no longer works ("is not supported") while the proposal sounds like we're not suggesting a breaking change. I assume the goal is merely to force people to make an explicit choice regarding the hash, instead of relying on the default (which we can't change)?

[vcsjones]

I assume the goal is merely to force people to make an explicit choice regarding the hash, instead of relying on the default (which we can't change)?

Correct. I tried to clarify that with "constructors with default hash algorithm or iterations" but if there is a better way to phrase this I would love to improve it.

[bartonjs]

Yeah, instead of

Rfc2898DeriveBytes constructors with default hash algorithm or iterations are obsolete and not supported. Use a constructor that accepts the hash algorithm and the number of iterations.

we probably want something like

The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.

[terrajobst]

I like that!

[terrajobst]

Video

• Looks good.
• We should change the wording to what Jeremy suggested:

  The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.

• We might want to consider calling out the defaults in the obsolete message so that people can apply the current defaults (having a fixer would be even better though)
• We should assign a custom diagnostic ID

[GrabYourPitchforks]

tl;dr: I'm good with these obsoletions and the proposed fixer which rewrites them using the newer overloads.

Words, words, words

I see this API as a fundamental building block. Cryptographic specs are usually written in terms of families of algorithms which have various configuration knobs. Sometimes the knobs should be tweaked depending on the various user scenario (CBC vs CTS padding modes for symmetric algorithms) or as a security / performance tradeoff (key length or iteration count).

IMO, .NET should not be in the business of prescribing defaults for cryptographic primitives. By setting such defaults, we're either lifting one scenario to be the One True Scenario™ that should be preferred above all others, or we're locking in a security / performance tradeoff that might only make sense at a snapshot in time. (The defaults in this API fall under this latter bucket.)

Ideally our basic cryptographic primitive APIs (including this!) should require callers to specify all configuration knobs as appropriate for their scenario. Yes, it's verbose, but it's the right thing to do to ensure that the caller is fully aware of what they're doing and that they understand they take full responsibility for its applicability to their scenario. This gatekeeps these APIs to an extent, but I am ok with that given that only knowledgeable people should be using the primitives directly.

By forcing callers to set parameters explicitly, we also make it easier for code reviewers or automated tools to audit the call sites for appropriateness. This could include mandating a minimum iteration count or banning certain algorithm families. We have such rules within Microsoft, for instance.

The vast majority of our users - those who are scenario-driven rather than trying to adhere to a particular protocol - would be better served by an opinionated crypto stack. Opinionated stacks are somewhat opaque in that they hide the complexity of choosing appropriate defaults, but they're usually much easier to use and expose only pit-of-success APIs. (The aspnet crypto stack is the best example of an opinionated crypto stack within .NET, but I'm also biased here, soooooo... 😃)

Given that this is a building block and not an opinionated API, I'm not terribly concerned with keeping it approachable. It should be as verbose as necessary to make the call site understandable, with appropriate documentation updates if needed.

[terrajobst]

Makes sense. Now I'm curious what your thoughts are on #53432 (comment) :-)

[danmoseley]

@vcsjones is this fixed by your #67158? if not, do you plan to do this or should we mark up for grabs?

[vcsjones]

@danmoseley there is still the analyzer aspect to this that I am tentatively going to start working on soon. If I don't pick it up soon then I will mark it up-for-grabs.

[vcsjones]

I'm unlikely to have time to learn how and implement a Roslyn analyzer for this in the short term. Marking up for grabs if someone is able to implement the analyzer piece.

[terrajobst]

I'm unlikely to have time to learn how and implement a Roslyn analyzer for this in the short term.

I don't think this requires a custom analyzer. It would just be something along those lines, no?

namespace System.Security.Cryptography;

public partial class Rfc2898DeriveBytes : DeriveBytes
{
    [Obsolete("<words>",
              DiagnosticId="<someid>",
              UrlFormat="https://aka.ms/<something>{0}")]
    public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations);
}

[vcsjones]

@terrajobst sorry, "analyzer" was not the right term. "Code fixer", the thingy that helps people move off the obsoleted constructors and on to the non-obsolete ones.

[terrajobst]

Ah, gotcha