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

Disallow control characters in user-id and password to be RFC-compliant. #55

Open
issuefiler opened this issue Dec 2, 2022 · 7 comments

Comments

@issuefiler
Copy link

var USER_PASS_REGEXP = /^([^:]*):(.*)$/

basic-auth/index.js

Lines 114 to 115 in e8a29f9

// decode user pass
var userPass = USER_PASS_REGEXP.exec(decodeBase64(match[1]))


Which, RFC 7617 forbids.

RFC 7617 — “Basic” HTTP Authentication Scheme

The user-id and password MUST NOT contain any control characters (see “CTL” in Appendix B.1 of RFC5234).

@dougwilson
Copy link
Contributor

dougwilson commented Dec 2, 2022

Hello, and thank you for your issue. You are correct regarding RFC 7617, although this module is written to RFC 2617, as this module was created prior to the existence of 7617.

It is possible that a future version of this module can update to instead follow RFC 7617 as a breaking change, though I don't see the exact purpose to make it more strict and break existing applications.

We could always add an option to the parser to choose with RFC to follow, would that be of interest to you?

@dougwilson dougwilson changed the title It erroneously allows control characters in user-id and password. Question about user-id and password. Dec 2, 2022
@dougwilson
Copy link
Contributor

I do see that TEXT is defined as follows, however:

       TEXT           = <any OCTET except CTLs,
                        but including LWS>

which looks like it also excludes certain CTL characters. I think we can release a 2.0 that adds this restriction. I need to think about if it should returned undefined or throw under this condition. Not sure if you have any thoughts on it or not.

@issuefiler
Copy link
Author

That’s right: RFC 2617 forbids all control characters but linear whitespace, and its successor, RFC 7617, simply forbids all of them including linear whitespace. Hence the original title of this issue.

I was just reporting it to inform you, not expecting an immediate “fix.” With this notice, you can do as you please with your package.

Releasing a new version with this restriction sounds good, by the way.

@issuefiler
Copy link
Author

If you don’t mind, may I change the title back?

@issuefiler issuefiler changed the title Question about user-id and password. Disallow control characters in user-id and password to be RFC-compliant. Dec 2, 2022
@issuefiler
Copy link
Author

And I believe it should return undefined, not throw, just like how invalid, malformed Authorization header field values are currently handled, so that it can be handled in the same manner (with 401 Unauthorized responses).

basic-auth/index.js

Lines 107 to 112 in e8a29f9

// parse header
var match = CREDENTIALS_REGEXP.exec(string)
if (!match) {
return undefined
}


RFC 9110 — HTTP semantics

11.4. Credentials

Upon receipt of a request for a protected resource that omits credentials, contains invalid credentials (e.g., a bad password) or partial credentials (e.g., when the authentication scheme requires more than one round trip), an origin server SHOULD send a 401 Unauthorized response that contains a WWW-Authenticate header field with at least one (possibly new) challenge applicable to the requested resource.

@issuefiler
Copy link
Author

I also doubt the need for “an option for the parser to choose between the RFC 2617 and RFC 7617 modes,” because,

compared to RFC 7617, the characters that RFC 2617 additionally supports in user-id and password are:

  • Carriage Return (␍/13),
  • Line Feed (␊/10),
  • and Horizontal Tab (␉/9),

which, innocent, sane users wouldn’t use in their usernames and passwords to sign in. Following the obsolete standard, RFC 2617, only benefits malicious attackers.

Internet Engineering Task Force (IETF)                        J. Reschke
Request for Comments: 7617                                    greenbytes
Obsoletes: 2617                                           September 2015

@UlisesGascon
Copy link
Member

Sorry I accidentally closed this issue

@UlisesGascon UlisesGascon reopened this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants