Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constrain X509ChainPolicy.UrlRetrievalTimeout to acceptable ranges #40578

Closed
vcsjones opened this issue Aug 8, 2020 · 4 comments · Fixed by #40676
Closed

Constrain X509ChainPolicy.UrlRetrievalTimeout to acceptable ranges #40578

vcsjones opened this issue Aug 8, 2020 · 4 comments · Fixed by #40676

Comments

@vcsjones
Copy link
Member

vcsjones commented Aug 8, 2020

The X509ChainPolicy.UrlRetrievalTimeout property is a TimeSpan that currently does not throw when given a value less than zero, or TotalMilliseconds larger than int.MaxValue. A negative value for this property behaves differently from Linux and Windows. Linux treats a negative value as-is and every network operation during chain building will time out (all time-budget has been used). Windows ends up treating the negative value as an unsigned value, so -1 ends up being treated as uint.MaxValue for example. However, Win32 appears to cap the value at 1 minute anyway, so all negative values are currently being treated as 1 minute.

I propose that the set for UrlRetrievalTimeout throw an ArgumentOutOfRangeException if it is less than zero or greater than TimeSpan.FromMilliseconds(int.MaxValue). Using a negative value likely isn't intentional and more likely a DateTime arithmetic mistake or a typo. Using a large value like TimeSpan.MaxValue may be done as an attempt to have no timeout.

This would be a breaking behavioral change, though a small one in my opinion.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Aug 8, 2020
@ghost
Copy link

ghost commented Aug 8, 2020

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

@vcsjones
Copy link
Member Author

vcsjones commented Aug 8, 2020

Do these kinds of changes generally need to go throw API review?

@vcsjones vcsjones changed the title Disallow X509ChainPolicy.UrlRetrievalTimeout to have negative values Constrain X509ChainPolicy.UrlRetrievalTimeout to acceptable ranges Aug 9, 2020
@bartonjs
Copy link
Member

@GrabYourPitchforks Do you have thoughts here? The "compat, compat everywhere" voice in my head says that we should just unify that negative numbers mean one minute. But as an API designer I agree that an ArgumentOutOfRangeException in the setter is what the original implementation should have had.

One possible answer is to wait another week and change it as the first break for 6; though I handwavily feel that it's better to break it in 5 than 6-preview.

@vcsjones
Copy link
Member Author

unify that negative numbers mean one minute

It's technically the max of whatever that version of Windows is willing to tolerate. I have no idea if the cap is say, 5 minutes on Windows 7 / 8, then negative number corresponds to 5 minutes.

@jeffhandley jeffhandley removed the untriaged New issue has not been triaged by the area owner label Aug 14, 2020
@jeffhandley jeffhandley added this to the 5.0.0 milestone Aug 14, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants