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

Digest challenge - realm fails with empty string #56369

Closed
camillo-toselli opened this issue Jul 27, 2021 · 20 comments · Fixed by #56455
Closed

Digest challenge - realm fails with empty string #56369

camillo-toselli opened this issue Jul 27, 2021 · 20 comments · Fixed by #56455
Labels
area-System.Net bug help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@camillo-toselli
Copy link
Contributor

camillo-toselli commented Jul 27, 2021

The issue is similar to #50283 but this time about realm key. For example:

	WWW-Authenticate : Digest realm="", nonce="NjBGRkMxNjUgY2FiN2YxZDM1MWM4ZDAyOTRiMmY2ZGVjOGMxMDY2Zjg=", algorithm="MD5", qop="auth"

will fail with error Nonce missing

RFC7616 says realm SHOULD contain al least the name of the server, but not MUST contain, so it doesn't exclude an empty realm

	This string should contain at least the name of the host performing the authentication
	and might additionally indicate the collection of users who might have access.

This lines of method Parse in class System.Net.Http.AuthenticationHelper.DigestResponse

	// Ensure value is valid.
	// Opaque and Domain can have empty string
	if (value == string.Empty &&
	   (!key.Equals(Opaque, StringComparison.OrdinalIgnoreCase) && !key.Equals(Domain, StringComparison.OrdinalIgnoreCase)))
	    break;

should be modified to allow empty Realm value

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jul 27, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jul 27, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

The issue is similar to #50283 but this time about realm key. For example:

WWW-Authenticate : Digest realm="", nonce="NjBGRkMxNjUgY2FiN2YxZDM1MWM4ZDAyOTRiMmY2ZGVjOGMxMDY2Zjg=", algorithm="MD5", qop="auth"

will fail with error Nonce missing

RFC7616 says realm SHOULD contain al least the name of the server, but not MUST contain, so it doesn't exclude an empty realm

This string should contain at least the name of the host performing the authentication
and might additionally indicate the collection of users who might have access.

This lines of method Parse in class System.Net.Http.AuthenticationHelper.DigestResponse

// Ensure value is valid.
// Opaque and Domain can have empty string
if (value == string.Empty &&
   (!key.Equals(Opaque, StringComparison.OrdinalIgnoreCase) && !key.Equals(Domain, StringComparison.OrdinalIgnoreCase)))
    break;

should be modified to allow empty Realm value

Author: camillo-toselli
Assignees: -
Labels:

area-System.Net, untriaged

Milestone: -

@karelz karelz added bug and removed untriaged New issue has not been triaged by the area owner labels Jul 27, 2021
@karelz karelz added this to the 7.0.0 milestone Jul 27, 2021
@karelz
Copy link
Member

karelz commented Jul 27, 2021

Triage: Seems legit. Are you interested in submitting PR for it?
Otherwise, moving to 7.0. Should be fairly straightforward to fix.

@karelz karelz added the help wanted [up-for-grabs] Good issue for external contributors label Jul 27, 2021
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Jul 28, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 28, 2021
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Jul 29, 2021
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Jul 29, 2021
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Jul 30, 2021
wfurt pushed a commit that referenced this issue Aug 3, 2021
* accept empty realm for digest auth (#56369)

* accept empty realm for digest auth (#56369)

* accept empty realm for digest auth (#56369)

* accept empty realm for digest auth (#56369)

Co-authored-by: Luca Bompani <luca.bompani@unibo.it>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 3, 2021
thaystg added a commit to thaystg/runtime that referenced this issue Aug 3, 2021
# By Camillo Toselli (1) and others
# Via GitHub
* origin/main:
  add RID for Debian 11 (dotnet#56789)
  [wasm] [debugger] Skip thread static field (dotnet#56749)
  Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
  Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
  accept empty realm for digest auth (dotnet#56369) (dotnet#56455)

# Conflicts:
#	src/mono/wasm/debugger/DebuggerTestSuite/BreakpointTests.cs
#	src/mono/wasm/debugger/DebuggerTestSuite/GetPropertiesTests.cs
thaystg added a commit to thaystg/runtime that referenced this issue Aug 4, 2021
…ger_proxy_attribute

* origin/main: (340 commits)
  add RID for Debian 11 (dotnet#56789)
  [wasm] [debugger] Skip thread static field (dotnet#56749)
  Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
  Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
  accept empty realm for digest auth (dotnet#56369) (dotnet#56455)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  ...
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Aug 11, 2021
* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

Co-authored-by: Luca Bompani <luca.bompani@unibo.it>
(cherry picked from commit b0cea40)
@karelz karelz modified the milestones: 7.0.0, 6.0.0 Aug 17, 2021
@bompani
Copy link

bompani commented Sep 15, 2021

Is it possible to backport this fix to release/5.0 branch too?

@karelz
Copy link
Member

karelz commented Sep 15, 2021

@bompani we could if there is enough demand. What is impact on you / your customers? How many are affected? How much? Why is it critical for your app?
Also note that 6.0 just released RC1 which is go-live license. Given that it is LTS and will be supported much longer than 5.0, is it an option for you to upgrade to 6.0?

@bompani
Copy link

bompani commented Sep 15, 2021

Our app is a wab service that manages the telephony system of the University of Bologna. It's one of the biggest private telephony network based on asterisk, with about 10000 phone lines.
Without this fix the web service is not able to invoke the web API of some desktop phone models that require digest authentication with an empty realm.
The web service persistence layer is based on an Oracle database accessed through Entity Framework Core. We will not be able to upgrade to .NET 6.0 until Oracle will release the Oracle EF Core 6 provider.

@karelz
Copy link
Member

karelz commented Sep 15, 2021

@bompani so, you are making new deployment to new customer(s) where you found out in production that it does not work? Is that correct?
Or did it work earlier and stopped working suddenly? (What triggered it in that case?)
How many customers / Desktop phone models are directly impacted by this bug?

@bompani
Copy link

bompani commented Sep 15, 2021

We upgraded our ws from dotnet core 3.1 to .net 5.0 but we will not be able to deploy the new ws in production until this issue will be resolved. This issue was not present in dotnet core 3.1.

@karelz
Copy link
Member

karelz commented Sep 17, 2021

Did you use have SocketsHttpHandler disabled on 3.1 by a chance? https://docs.microsoft.com/en-us/dotnet/api/system.net.http.socketshttphandler?view=net-5.0#remarks

@bompani
Copy link

bompani commented Sep 20, 2021

I apologize because I reported a wrong information: the production version of out ws is developed with .NET Framework 4.5 not .Net core 3.1. Before the porting of our ws to .NET 5.0 we developed an intermediate version with .NET core 3.1 but I don't kwow if the digest authentication was working in that version.

bompani pushed a commit to camillo-toselli/runtime that referenced this issue Sep 21, 2021
* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

Co-authored-by: Luca Bompani <luca.bompani@unibo.it>
(cherry picked from commit b0cea40)
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Oct 13, 2021
* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

Co-authored-by: Luca Bompani <luca.bompani@unibo.it>
(cherry picked from commit b0cea40)
@bompani
Copy link

bompani commented Oct 29, 2021

Are there any chances that this fix will be merged into the 5.0 branch? Thanks

@karelz
Copy link
Member

karelz commented Nov 4, 2021

@bompani yes, there is still a chance -- I kicked off the servicing process in PR #61203

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 4, 2021
@karelz
Copy link
Member

karelz commented Nov 4, 2021

@bompani will you be able to validate on private (unsigned) binaries that the fix addresses your problem?

@bompani
Copy link

bompani commented Nov 5, 2021

Yes I'm able to validate on private binaries. Can I download the PR buld or have I to build it myself?

@karelz
Copy link
Member

karelz commented Nov 5, 2021

Great!
Building it yourself is of course an option.

The PR has build artifacts - libraries_bin_* e.g.:

Grab System.Net.Http.dll (in directory bin\runtime\net5.0-Windows_NT-Debug-x64 of the zip) and replace it in your local installation (tip: make a copy of the original file first). The debug build should work with locally-installed release build, but I am not 100% sure -- if you hit any problems, let me know (I would build binaries you need locally - I would need the OS/bitness info you need), or you can compile it yourself if you are set up to do that.
Run the repro before replacing the binary to make sure it repros and then after you replace the binary to make sure it works correctly with the new bits.

@bompani
Copy link

bompani commented Nov 5, 2021

I can confirm that the PR build resolves our problem.
Thanks.

@karelz
Copy link
Member

karelz commented Nov 5, 2021

Thanks for validation @bompani!
Which OS / architecture did you validate? Did you use the build artifacts, or your local build?

@bompani
Copy link

bompani commented Nov 5, 2021

I validated my local build of branch backport/pr-56455-to-release/5.0 on Windows10/x64

@karelz
Copy link
Member

karelz commented Nov 5, 2021

Thanks for confirmation @bompani!

Anipik pushed a commit that referenced this issue Nov 8, 2021
* accept empty realm for digest auth (#56369)

* accept empty realm for digest auth (#56369)

* accept empty realm for digest auth (#56369)

* accept empty realm for digest auth (#56369)

Co-authored-by: Luca Bompani <luca.bompani@unibo.it>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 8, 2021
@karelz
Copy link
Member

karelz commented Nov 9, 2021

The bug is fixed:

@karelz karelz modified the milestones: 6.0.0, 5.0.x Nov 9, 2021
bompani pushed a commit to camillo-toselli/runtime that referenced this issue Nov 9, 2021
* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

* accept empty realm for digest auth (dotnet#56369)

Co-authored-by: Luca Bompani <luca.bompani@unibo.it>
(cherry picked from commit b0cea40)
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants