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

imap: Provide method to disable SASL if it is advertised #10041

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

kop316
Copy link
Contributor

@kop316 kop316 commented Dec 6, 2022

Hello!

There is no method to be able to use plain LOGIN (Plain LOGIN vs. SASL LOGIN) if an IMAP server advertises SASL capabilities. However, this may be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=" looks to be the correct way to specify what authenication method to use, regardless of SASL or not.

Since https://www.rfc-editor.org/rfc/rfc3501.html#section-6.1.1 provides no way to have no SASL mechanisms, "XNONE" looks to be the best RFC compliant way of doing it.

While this does provide a method for cleartext password being sent, a user must expliciticly call AUTH=XNONE to excerise this.

I should note I originally thought of adding this in Curl_sasl_parse_url_auth_option() to add a more general way of disabling SASL authentication (it did not break any scripts), but I wasn't sure which would be more appropriate.

@kop316 kop316 force-pushed the wip/sasl branch 2 times, most recently from e5976e9 to 089f431 Compare December 6, 2022 04:10
@bagder bagder added the IMAP label Dec 6, 2022
@bagder
Copy link
Member

bagder commented Dec 6, 2022

I'm totally fine with doing it this way. Two things:

  • This needs to be documented.
  • Why XNONE and not just NONE?

@kop316
Copy link
Contributor Author

kop316 commented Dec 6, 2022

I'm totally fine with doing it this way. Two things:

Sounds good!

* This needs to be documented.

That is fine! EDIT: I added the documentation in another commit, but I can squash it too if that is desirable. I assume I should add something like IMAP AUTH-XNONE was added in curl $VERSION, and I assume $VERSION is 7.87.0?

* Why `XNONE` and not just `NONE`?

In https://www.rfc-editor.org/rfc/rfc3501.html :

capability      = ("AUTH=" auth-type) / atom
                    ; New capabilities MUST begin with "X" or be
                    ; registered with IANA as standard or
                    ; standards-track

I read that to mean that since NONE is not an IANA standard (they don't seem to have any reserved words for this), I needed to put an X in front of it. However, if you disagree and prefer NONE, that is fine too and I can change it.

@kop316
Copy link
Contributor Author

kop316 commented Dec 11, 2022

Hello!

I just wanted to follow up with this. I believe I have the documentation added (though that's pending review). Should I also change XNONE to NONE? I don't mind that, I just wasn't sure if my reasoning in the previous comment was correct or not.

@kop316
Copy link
Contributor Author

kop316 commented Jan 3, 2023

Hello and happy new year!

I just wanted to follow up on this again. I was waiting for feedback on the comments here: #10041 (comment)

to do any changes, and I wanted to check in to see what your thoughts were.

@craftyguy
Copy link

@kop316 thanks for this patch, since I believe it'll fix some authentication problems with some poorly-implemented cell carrier servers we have to deal with in postmarketOS / mobile linux distros...

I'm a little hesitant to pick this patch and ship it, since anything around authentication is automatically "scary" for me due to my lack of understanding around these things... so I'm waiting until this is accepted/merged before doing so.

@bagder I know you probably have other priorities than this one, but any help for @kop316 to get this in an acceptable state would be greatly appreciated!

@kop316 kop316 force-pushed the wip/sasl branch 2 times, most recently from 03c9815 to 8a9cda3 Compare February 28, 2023 01:21
@kop316
Copy link
Contributor Author

kop316 commented Feb 28, 2023

I'm totally fine with doing it this way. Two things:

* This needs to be documented.

* Why `XNONE` and not just `NONE`?

Hello,

Based on this, I added documentation and revert it to NONE. Please let me know if there is anything else you would like me to do for it.

EDIT: I am not sure why 126 is failing for FreeBSD 13.2, since that is for FTP and I didn't do anything with it? I am thinking it could be a transient issue but I am not sure how to start another run for it without triggering all of them.

@adamplumb
Copy link

I just want to chime in and add my support for this or this other PR for fixing out-of-the-box voicemail issues on mobile linux.

@jay
Copy link
Member

jay commented Apr 10, 2023

The other PR is held up by a security concern and my lack of SASL knowledge. If AUTH NONE is used I would think then there is no authentication? I don't see 'none' in the rfcs

@monnerat
Copy link
Contributor

I really prefer this way of resolving the problem rather than the fallback of #10027: the later causes a very fast double authentication attempt that may appear to the server as an attack and finally locks the user and/or ip.

The problem is real: there is no way to select a non-SASL login method if some SASL are advertised by the server. It exists since introduction of SASL in curl and probably also affects POP3, SMTP and LDAP.

The ideal solution would be to consider ALL authentication methods (SASL + protocol login + APOP, etc) without discrimination and allow giving them in the AUTH= parameter, but I currently don't have time for that change, although plenty of ideas.

In the meantime I would prefer something like AUTH=XLOGIN and reserve the NONE/XNONE in case a "no-authentication" feature is needed some day.

Please note that POP3 URL requires a non-SASL mechanism name to begin with a '+' (RFC 2384 page 2), but there is no such restriction for IMAP (RFC 2362). XLOGIN or +LOGIN seem good possible mechanism names for IMAP, as LOGIN is already used by SASL The former has the advantage to be POP3-compatible if introduced some day.

@kop316
Copy link
Contributor Author

kop316 commented Apr 11, 2023

@jay @monnerat

This PR will solve the underlying issue I am facing, so i am fine with this instead of the other one.

Reading though #10041 (comment) , if I change NONE to XLOGIN or +LOGIN, would this PR be good for merging? I am admittedly confused, as it sounded like +LOGIN may be better for POP from:

POP3 URL requires a non-SASL mechanism name to begin with a '+' (RFC 2384 page 2)

But

The former has the advantage to be POP3-compatible if introduced some day.

Implied that XLOGIN is preferable.

Please let me know which one you would prefer and I will change as appropriate.

Thank you!

@monnerat
Copy link
Contributor

Please let me know which one you would prefer and I will change as appropriate.

I'm in favor of +LOGIN: this will allow a more hortogonal option string between protocols (we already have +APOP in POP3). But others' opinion matter, of course.

@github-actions github-actions bot added the tests label Apr 11, 2023
@kop316
Copy link
Contributor Author

kop316 commented Apr 11, 2023

Please let me know which one you would prefer and I will change as appropriate.

I'm in favor of +LOGIN: this will allow a more hortogonal option string between protocols (we already have +APOP in POP3). But others' opinion matter, of course.

I personally do not have a strong opinion, as they would all work for me equally as well. I went ahead and changed NONE to +LOGIN in another commit. Perhaps in a few days if no one chimes in I just squash for merging?

@jay
Copy link
Member

jay commented Apr 14, 2023

@monnerat does this look correct to you now, if SASL_AUTH_NONE is set to use IMAP_TYPE_CLEARTEXT; because SASL_AUTH_NONE is used when prefs are reset so should it be defaulting to cleartext in that case?

@monnerat
Copy link
Contributor

does this look correct to you now,

It looks like, yes.

if SASL_AUTH_NONE is set to use IMAP_TYPE_CLEARTEXT; because SASL_AUTH_NONE is used when prefs are reset so should it be defaulting to cleartext in that case?

This is the center of the initial problem and I'm not the father of this code. Probably some more testing should be done on it, but as stated earlier, I don't have time for it right now. However I'm quite confident but I won't take this responsibility.

@kop316
Copy link
Contributor Author

kop316 commented Apr 14, 2023

This is the center of the initial problem and I'm not the father of this code. Probably some more testing should be done on it, but as stated earlier, I don't have time for it right now. However I'm quite confident but I won't take this responsibility.

Sorry, just so I am clear, are you saying more testing needs to be done on the MR, or more testing needs to be done on the initial problem to completely fix it?

I just want to make sure you are not needing anything else from me.

@monnerat
Copy link
Contributor

Sorry, just so I am clear, are you saying more testing needs to be done on the MR, or more testing needs to be done on the initial problem to completely fix it?

I'm just saying the PR looks good, but as this part of libcurl code is not very clear, I cannot predict possible border effects :-)

@kop316
Copy link
Contributor Author

kop316 commented Apr 14, 2023

Ahh, fair enough! I could never get the code to reach the line here: https://github.com/curl/curl/pull/10041/files#diff-9dd142f0e3406c05d5878d67ca2460c070b4e8f684abd673e6e38f58493cb008L1953 . That is why I manually changed it how I did. However if there is a desire to change it, please let me know and I will.

EDIT: One thing that could be done too is adding something like SASL_AUTH_DISABLE to Curl_sasl_parse_url_auth_option() to make this a bit more explicit. That way this MR would not Alias with SASL_AUTH_NONE. This would also help to make disabling SASL more available for e.g. POP3, SMTP, LDAP (though I think actually adding that is outside the realm of this particular MR...) This was another option I alluded to in this comment: #10041 (comment) .

Would this option be more desirable?

@kop316
Copy link
Contributor Author

kop316 commented Apr 21, 2023

@jay @monnerat Hello! I just wanted to follow up with this.

It sounded like the PR is good, but I also coded up this method to do the same thing: master...kop316:curl:wip/sasl2 and it passes CI as well in case there was a worry about side effects.

@monnerat
Copy link
Contributor

I'm not OK with the other:

#define SASL_AUTH_DISABLE 0xff00

These are supposed to be a mechanisn bits mask and not a specific token value.
It is equivalent of SASL_MECH_OAUTHBEARER | SASL_MECH_SCRAM_SHA_1 | SASL_MECH_SCRAM_SHA_256 | some_other_yet_undefined_mechanisms and will lock our inevitable future use of the latter in an orthogonal way.

@kop316
Copy link
Contributor Author

kop316 commented Apr 21, 2023

I'm not OK with the other:

#define SASL_AUTH_DISABLE 0xff00

These are supposed to be a mechanisn bits mask and not a specific token value. It is equivalent of SASL_MECH_OAUTHBEARER | SASL_MECH_SCRAM_SHA_1 | SASL_MECH_SCRAM_SHA_256 | some_other_yet_undefined_mechanisms and will lock our inevitable future use of the latter in an orthogonal way.

That's fair, thanks for explaining! It sounds like I will stick to the original PR then.

@jay
Copy link
Member

jay commented Apr 29, 2023

@kop316 I just pushed a commit to your branch that adds a separate 'prefer_login' variable that will determine whether or not to set IMAP_TYPE_CLEARTEXT. @monnerat are there any problems with this approach? If so then Chris can just force push back to where it was.

@vszakats
Copy link
Member

vszakats commented May 5, 2023

Can we consider limiting this fallback to IMAPS connections only?

My concern is apps enabling this under the hood and leaking passwords in cleartext over the network.

@bagder
Copy link
Member

bagder commented May 5, 2023

IMAPS connections only

I think that might be clever.

Just a caution around that: I imagine that the larger use of TLS for IMAP is done with STARTTLS, so they are actually using an IMAP:// URL but with CURLOPT_USE_SSL set. So they start out clear text but upgrade into TLS. It unfortunately makes it a little harder for libcurl to know if it is an "IMAPS" connection at any given point.

@monnerat
Copy link
Contributor

monnerat commented May 5, 2023

Can we consider limiting this fallback to IMAPS connections only?
My concern is apps enabling this under the hood and leaking passwords in cleartext over the network.

That's what PR #8295 is intended to prevent, in a more global scope as SASL may also transmit clear passwords.

@vszakats
Copy link
Member

vszakats commented May 5, 2023

STARTTLS is trivial to downgrade-attack, so maybe it in this context we can consider is as cleartext IMAP. UPDATE: ..when there is way to tell if the connection has already been upgraded to TLS.

@kop316
Copy link
Contributor Author

kop316 commented May 5, 2023

So I am clear, is the PR good as is, or is there work that is desired?

@vszakats
Copy link
Member

vszakats commented May 5, 2023

IMO this adds a high risk for leaking email account passwords. Considering that email account passwords are one the most valuable ones (or worst ones to have leaked), I think we should make it dependent of having active TLS on the connection. Even if behind a flag, it will be enabled invisibly by apps building on curl and pose a risk to end-users. This seems like a too high price to pay to fix compatibility with buggy servers.

@monnerat
Copy link
Contributor

monnerat commented May 5, 2023

IMO this adds a high risk for leaking email account passwords.

Yes and no !

  • Yes because AUTH=+LOGIN does require a cleartext password authentication.
  • No because using an AUTH=LOGIN SASL mechanism is not better. In addition it can be selected automatically if it's the only SASL mechanism advertised by the server.
  • No again, because if a server does not advertise SASL, IMAP LOGIN is used automatically.
  • An still no, because AUTH=+LOGIN must be requested explicitly if SASL is available.

IMO, disabling cleartext password and using SASL mechanism or not should not be bound. The above mentioned PR treats this (disjoint) problem globally, It is currently awaiting your vote and/or comment. Maybe it should be implemented the other way round (i.e.: disabling cleartext password by default) at the cost of backwards incompatibility but the topic comes periodically in PR comments and mailing list, so something equivalent should be implemented to definitively close this chapter. I really don't understand why #8295 has not yet been adopted (or requested for changes) with regards to all these recurrent password security gripes.

@jay
Copy link
Member

jay commented May 6, 2023

I disagree, no more work is needed. This PR adds an option to specifically use plaintext LOGIN instead of server specified SASL. If the user sets it as a login option then that is what we give them.

@vszakats
Copy link
Member

vszakats commented May 7, 2023

Related news from a few days ago: https://mailbox.org/en/post/mailbox-org-discovers-unencrypted-password-transmission-in-mymail (on HN: https://news.ycombinator.com/item?id=35845308)

In this case it was the lack of TLS + one of the variations of cleartext password. Adding more ways to achieve this doesn't help.

jay pushed a commit to kop316/curl that referenced this pull request Jun 6, 2023
- Implement AUTH=+LOGIN to prefer plaintext LOGIN over SASL auth.

Prior to this change there was no method to be able to fallback to LOGIN
if an IMAP server advertises SASL capabilities. However, this may be
desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify
what authenication method to use, regardless of SASL or not.

Closes curl#10041
@kop316
Copy link
Contributor Author

kop316 commented Jun 8, 2023

Hello,

Given that we are in the feature window for the next curl release, I wanted to check back in to see if there is anything else needed from me for this.

Thank you!

jay pushed a commit to kop316/curl that referenced this pull request Jun 15, 2023
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fallback to LOGIN
if an IMAP server advertises SASL capabilities. However, this may be
desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
jay pushed a commit to kop316/curl that referenced this pull request Jun 15, 2023
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fall back to
LOGIN if an IMAP server advertises SASL capabilities. However, this may
be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
jay pushed a commit to kop316/curl that referenced this pull request Jun 15, 2023
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fall back to
LOGIN if an IMAP server advertises SASL capabilities. However, this may
be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
jay pushed a commit to kop316/curl that referenced this pull request Jun 15, 2023
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fall back to
LOGIN if an IMAP server advertises SASL capabilities. However, this may
be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fall back to
LOGIN if an IMAP server advertises SASL capabilities. However, this may
be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
@jay jay closed this in 64aefea Jun 16, 2023
@jay jay merged commit 64aefea into curl:master Jun 16, 2023
136 of 149 checks passed
@jay
Copy link
Member

jay commented Jun 16, 2023

Thanks

@jay jay removed the feature-window A merge of this requires an open feature window label Jun 16, 2023
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fall back to
LOGIN if an IMAP server advertises SASL capabilities. However, this may
be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
- Implement AUTH=+LOGIN for CURLOPT_LOGIN_OPTIONS to prefer plaintext
  LOGIN over SASL auth.

Prior to this change there was no method to be able to fall back to
LOGIN if an IMAP server advertises SASL capabilities. However, this may
be desirable for e.g. a misconfigured server.

Per: https://www.ietf.org/rfc/rfc5092.html#section-3.2

";AUTH=<enc-auth-type>" looks to be the correct way to specify what
authenication method to use, regardless of SASL or not.

Closes curl#10041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants