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: doesn't quote atoms if they contain quotes! #1902

Closed
imilli opened this Issue Sep 21, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@imilli

imilli commented Sep 21, 2017

There is a bug for imap protocol:
I did this:
Only with these symbols (){ %*] can add double quotes at password both, if passworld includes symbol " or \ but exclude (){ %*] , it doesn't. This will cause some mail servers(for example: coremail server) to fail to access properly.
I expected the following:
The password should be added symbol " to both ends, whether or not it contains special symbols.
in file imap.c , function imap_atom(), set bool others_exists = TRUE, will resolve it.
libcurl version : 7.55.1
operating system : linux

@jay jay added the IMAP label Sep 21, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

Only with these symbols (){ %] can add double quotes at password both, if passworld includes symbol " or \ but exclude (){ %] , it doesn't.

Are you saying that the password you sent contained one of those symbols but curl didn't send it quoted? I just tested with -u user*:secret{ and it seemed to send them quoted just fine.

Can you show us a protocol trace of what curl did and what you expected curl to send instead?

Member

bagder commented Sep 21, 2017

Only with these symbols (){ %] can add double quotes at password both, if passworld includes symbol " or \ but exclude (){ %] , it doesn't.

Are you saying that the password you sent contained one of those symbols but curl didn't send it quoted? I just tested with -u user*:secret{ and it seemed to send them quoted just fine.

Can you show us a protocol trace of what curl did and what you expected curl to send instead?

bagder added a commit that referenced this issue Sep 21, 2017

tests: make the imap server not verify user+password
... as the test cases themselves do that and it makes it easier to add
crazy test cases.

Test 800 updated to use user name + password that needs quoting.

Test 856 updated to trigger an auth fail differently.

Ref: #1902
@imilli

This comment has been minimized.

Show comment
Hide comment
@imilli

imilli Sep 21, 2017

you can use password following:
admin"123
The password sent by curl is admin"123, but the correct one should be "admin\"123"
if the password includs one of those symbols (){ %], for example following:
admin"{123, the curl will sent the password that is "admin\"{123".

imilli commented Sep 21, 2017

you can use password following:
admin"123
The password sent by curl is admin"123, but the correct one should be "admin\"123"
if the password includs one of those symbols (){ %], for example following:
admin"{123, the curl will sent the password that is "admin\"{123".

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Sep 21, 2017

Member

@bagder What he's referring to is imap_atom the string is only wrapped in double quotes when escape_only param is true false and some atom-special was found which sets others_exists is true , and then that only happens for the atom_specials characters (){ %*].

curl/lib/imap.c

Lines 1745 to 1754 in 8839c05

else if(!escape_only) {
const char *p3 = atom_specials;
while(*p3 && !others_exists) {
if(*p1 == *p3)
others_exists = TRUE;
p3++;
}
}

According to the IMAP RFC:

atom-specials   = "(" / ")" / "{" / SP / CTL / list-wildcards /
                  quoted-specials / resp-specials
list-wildcards  = "%" / "*"
quoted-specials = DQUOTE / "\"
resp-specials   = "]"

(CTL is %x00-1F / %x7F and DQUOTE is double quote).

I can only find one reference to atom-specials in the entire RFC:

   1)    Any character which is one of the atom-specials (see the Formal
         Syntax) will require that the mailbox name be represented as a
         quoted string or literal.

It would seem, at least in that case, that escape_only when atom-specials are present may not be appropriate unless the calling function is going to quote the string.

I think we could expand our character set of atom-specials at least. Whether the password should be sent as admin"123 admin\"123 or "admin\"123" I'm not seeing it.

Member

jay commented Sep 21, 2017

@bagder What he's referring to is imap_atom the string is only wrapped in double quotes when escape_only param is true false and some atom-special was found which sets others_exists is true , and then that only happens for the atom_specials characters (){ %*].

curl/lib/imap.c

Lines 1745 to 1754 in 8839c05

else if(!escape_only) {
const char *p3 = atom_specials;
while(*p3 && !others_exists) {
if(*p1 == *p3)
others_exists = TRUE;
p3++;
}
}

According to the IMAP RFC:

atom-specials   = "(" / ")" / "{" / SP / CTL / list-wildcards /
                  quoted-specials / resp-specials
list-wildcards  = "%" / "*"
quoted-specials = DQUOTE / "\"
resp-specials   = "]"

(CTL is %x00-1F / %x7F and DQUOTE is double quote).

I can only find one reference to atom-specials in the entire RFC:

   1)    Any character which is one of the atom-specials (see the Formal
         Syntax) will require that the mailbox name be represented as a
         quoted string or literal.

It would seem, at least in that case, that escape_only when atom-specials are present may not be appropriate unless the calling function is going to quote the string.

I think we could expand our character set of atom-specials at least. Whether the password should be sent as admin"123 admin\"123 or "admin\"123" I'm not seeing it.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

you can use password following:
admin"123
The password sent by curl is admin"123, but the correct one should be "admin"123"

This doesn't match my test. I've modified test 800 to use this command line:

curl 'imap://%HOSTIP:%IMAPPORT/800/;UID=1' -u '"user*:sec"ret{'

It makes curl send this line:

A002 LOGIN "\"user*" "sec\"ret{" (followed by CRLF)

Surely the double quote used in the atom needs to be backslash-escaped ?

Member

bagder commented Sep 21, 2017

you can use password following:
admin"123
The password sent by curl is admin"123, but the correct one should be "admin"123"

This doesn't match my test. I've modified test 800 to use this command line:

curl 'imap://%HOSTIP:%IMAPPORT/800/;UID=1' -u '"user*:sec"ret{'

It makes curl send this line:

A002 LOGIN "\"user*" "sec\"ret{" (followed by CRLF)

Surely the double quote used in the atom needs to be backslash-escaped ?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

@jay wrote:

It would seem, at least in that case, that escape_only when atom-specials are present may not be appropriate unless the calling function is going to quote the string.

The only current user of imap_atom() that sets the escape_only parameter to TRUE is within imap_perform_list, and it does quote the string itself...

Member

bagder commented Sep 21, 2017

@jay wrote:

It would seem, at least in that case, that escape_only when atom-specials are present may not be appropriate unless the calling function is going to quote the string.

The only current user of imap_atom() that sets the escape_only parameter to TRUE is within imap_perform_list, and it does quote the string itself...

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

Aha!

When using a string like "user it creates an atom like \"user that isn't quoted!

Member

bagder commented Sep 21, 2017

Aha!

When using a string like "user it creates an atom like \"user that isn't quoted!

@imilli

This comment has been minimized.

Show comment
Hide comment
@imilli

imilli Sep 21, 2017

To sum up, if the username or password is included " or \, not included one of (){ %*] , curl will sends password without double quotes.
if like this, It will not be compatible with some mail system.e.g coremail mail server.
i tested some mail client ,e.g foxmail, if send password like 111111, the password "111111" will be sent.

imilli commented Sep 21, 2017

To sum up, if the username or password is included " or \, not included one of (){ %*] , curl will sends password without double quotes.
if like this, It will not be compatible with some mail system.e.g coremail mail server.
i tested some mail client ,e.g foxmail, if send password like 111111, the password "111111" will be sent.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

i tested some mail client ,e.g foxmail, if send password like 111111, the password "111111" will be sent.

Sure, I'm sure some clients default to always quoting but that's a different thing. The IMAP protocol doesn't mandate quoting for a string like 111111.

Member

bagder commented Sep 21, 2017

i tested some mail client ,e.g foxmail, if send password like 111111, the password "111111" will be sent.

Sure, I'm sure some clients default to always quoting but that's a different thing. The IMAP protocol doesn't mandate quoting for a string like 111111.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

Maybe this?

--- a/lib/imap.c
+++ b/lib/imap.c
@@ -1795,20 +1795,20 @@ static char *imap_atom(const char *str, bool escape_only)
   /* Does the input contain any "atom-special" characters? */
   if(!backsp_count && !quote_count && !others_exists)
     return strdup(str);
 
   /* Calculate the new string length */
-  newlen = strlen(str) + backsp_count + quote_count + (others_exists ? 2 : 0);
+  newlen = strlen(str) + backsp_count + quote_count + (escape_only ? 0 : 2);
 
   /* Allocate the new string */
   newstr = (char *) malloc((newlen + 1) * sizeof(char));
   if(!newstr)
     return NULL;
 
   /* Surround the string in quotes if necessary */
   p2 = newstr;
-  if(others_exists) {
+  if(!escape_only) {
     newstr[0] = '"';
     newstr[newlen - 1] = '"';
     p2++;
   }
 
Member

bagder commented Sep 21, 2017

Maybe this?

--- a/lib/imap.c
+++ b/lib/imap.c
@@ -1795,20 +1795,20 @@ static char *imap_atom(const char *str, bool escape_only)
   /* Does the input contain any "atom-special" characters? */
   if(!backsp_count && !quote_count && !others_exists)
     return strdup(str);
 
   /* Calculate the new string length */
-  newlen = strlen(str) + backsp_count + quote_count + (others_exists ? 2 : 0);
+  newlen = strlen(str) + backsp_count + quote_count + (escape_only ? 0 : 2);
 
   /* Allocate the new string */
   newstr = (char *) malloc((newlen + 1) * sizeof(char));
   if(!newstr)
     return NULL;
 
   /* Surround the string in quotes if necessary */
   p2 = newstr;
-  if(others_exists) {
+  if(!escape_only) {
     newstr[0] = '"';
     newstr[newlen - 1] = '"';
     p2++;
   }
 

bagder added a commit that referenced this issue Sep 21, 2017

tests: make the imap server not verify user+password
... as the test cases themselves do that and it makes it easier to add
crazy test cases.

Test 800 updated to use user name + password that need quoting.

Test 856 updated to trigger an auth fail differently.

Ref: #1902

bagder added a commit that referenced this issue Sep 21, 2017

imap: quote atoms properly when escaping characters
Updates test 800 to verify

Fixes #1902

@bagder bagder changed the title from A bug for imap to IMAP: doesn't quote atoms if they contain quotes! Sep 21, 2017

@imilli

This comment has been minimized.

Show comment
Hide comment
@imilli

imilli Sep 21, 2017

if you change it like this,
/* Does the input contain any "atom-special" characters? */
if(!backsp_count && !quote_count && !others_exists)
return strdup(str);
simple password will not add " .

imilli commented Sep 21, 2017

if you change it like this,
/* Does the input contain any "atom-special" characters? */
if(!backsp_count && !quote_count && !others_exists)
return strdup(str);
simple password will not add " .

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 21, 2017

Member

simple password will not add " .

Correct, and that's on purpose. Are you saying that "coremail" requires the password to be quoted? If so, isn't that then a bug in the server that will cause problems to more clients than just curl?

Member

bagder commented Sep 21, 2017

simple password will not add " .

Correct, and that's on purpose. Are you saying that "coremail" requires the password to be quoted? If so, isn't that then a bug in the server that will cause problems to more clients than just curl?

bagder added a commit that referenced this issue Sep 22, 2017

tests: make the imap server not verify user+password
... as the test cases themselves do that and it makes it easier to add
crazy test cases.

Test 800 updated to use user name + password that need quoting.

Test 856 updated to trigger an auth fail differently.

Ref: #1902

@bagder bagder closed this in 3b05f79 Sep 22, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.