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

Crashes with libssh2_userauth_keyboard_interactive_ex #6691

Closed
MonkeybreadSoftware opened this issue Mar 4, 2021 · 11 comments
Closed

Crashes with libssh2_userauth_keyboard_interactive_ex #6691

MonkeybreadSoftware opened this issue Mar 4, 2021 · 11 comments

Comments

@MonkeybreadSoftware
Copy link
Contributor

@MonkeybreadSoftware MonkeybreadSoftware commented Mar 4, 2021

We got reports about crashes with recent CURL update and libssh2_userauth_keyboard_interactive_ex function.

Doing SFTP with LibSSH2 and OpenSSL causes a crash if no username/password is set.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_platform.dylib 0x00007fff6a342e52 _platform_strlen + 18
1 libsystem_c.dylib 0x00007fff6a1f2cd8 strdup + 18
2 app 0x0000000113ced7c0 kbd_callback + 40
3 app 0x0000000113ae6032 libssh2_userauth_keyboard_interactive_ex + 1698
4 app 0x0000000113ce99fd ssh_statemach_act + 2082
5 app 0x0000000113ce9087 ssh_multi_statemach + 39
6 app 0x0000000113cc2648 multi_runsingle + 1079
7 app 0x0000000113cc2113 curl_multi_perform + 129
8 app 0x0000000113ca82ef curl_easy_perform + 309

Not sure what change in 7.75 causes this.

Sadly it looks like strdup and strlen are set.

Anyone seen this, too?

@MonkeybreadSoftware
Copy link
Contributor Author

@MonkeybreadSoftware MonkeybreadSoftware commented Mar 4, 2021

We patched libssh2.c ourselves with those changes to check for NULL pointer:

In kbd_callback function:

static void
kbd_callback(const char *name, int name_len, const char *instruction,
             int instruction_len, int num_prompts,
             const LIBSSH2_USERAUTH_KBDINT_PROMPT *prompts,
             LIBSSH2_USERAUTH_KBDINT_RESPONSE *responses,
             void **abstract)

if(num_prompts == 1) { responses[0].text = conn->passwd ? strdup(conn->passwd) : NULL; responses[0].length = conn->passwd ? curlx_uztoui(strlen(conn->passwd)) : 0; }

So we added check with ? For whether variable is NULL.

And later in ssh_statemach_act:

static CURLcode ssh_statemach_act(struct Curl_easy *data, bool *block)

We also got NULL checks before passing values:

      rc = libssh2_userauth_password_ex(sshc->ssh_session, 
                                        conn->user ? conn->user : "",
                                        conn->user ? curlx_uztoui(strlen(conn->user)) : 0,
                                        conn->passwd ? conn->passwd: "",
                                        conn->passwd ? curlx_uztoui(strlen(conn->passwd)) : 0,
                                        NULL);

In may be better to earlier check this once and set to empty string if it is NULL.

Could someone confirm the problem and maybe include the changes for future releases?

@jay
Copy link
Member

@jay jay commented Mar 4, 2021

Note there are several other places in libcurl's libssh2.c where it assumes conn->user is not NULL and does strlen(conn->user). libssh2 code in libssh2_userauth_password_ex passes around username and password parameters to functions that check for NULL, though I notice the documentation doesn't explicitly state they may be NULL.

For the keyboard callback it looks like we are assuming not NULL as well for conn->passwd. The LIBSSH2_USERAUTH_KBDINT_RESPONSE is not documented, though it is used in a number of examples. I'm not sure of the right behavior, and none of the examples I saw set response to NULL.

/cc @willco007

@MonkeybreadSoftware
Copy link
Contributor Author

@MonkeybreadSoftware MonkeybreadSoftware commented Mar 4, 2021

I got the crashes here by using just an sftp:// URL and did not specify username and password.
Seems like the default value for username and password is NULL.

@jay
Copy link
Member

@jay jay commented Mar 4, 2021

Yup it's a problem but I don't whether it's right to pass them as an empty string or NULL, that's why I cc'd one of the libssh2 developers.

@willco007
Copy link

@willco007 willco007 commented Mar 4, 2021

I'm not super familiar with this code, but in a quick audit of the libssh2 code, it looks safe to pass NULL for user/password.

@jay
Copy link
Member

@jay jay commented Mar 4, 2021

Thanks. @MonkeybreadSoftware do you want to submit a PR?

@MonkeybreadSoftware
Copy link
Contributor Author

@MonkeybreadSoftware MonkeybreadSoftware commented Mar 5, 2021

I am trying here:

#6694

Although I would like to know if someone has an idea what changed within the last release or two, which could break this.

@jay
Copy link
Member

@jay jay commented Mar 5, 2021

#6694

That uses empty strings instead of NULL, but as Will noted you can pass NULL. Basically all the strlen(conn->user) or strlen(conn->passwd) need to be checked before passing to strlen and I think that's it, unless you are getting a null deref inside libssh2.

Although I would like to know if someone has an idea what changed within the last release or two, which could break this.

I don't know that anything did. You could do a bisect to find that out.

@bagder
Copy link
Member

@bagder bagder commented Mar 6, 2021

Without checking, I suspect I might've caused this with 46620b9... ?

@MonkeybreadSoftware
Copy link
Contributor Author

@MonkeybreadSoftware MonkeybreadSoftware commented Mar 6, 2021

Well, used there be a place which ensured ->user and ->passwd where always non NULL?
if such a line existed it may have been removed in recent changes.

@bagder
Copy link
Member

@bagder bagder commented Mar 8, 2021

I think it is better to have local code deal with that, to properly allow separation between no user name and a blank user name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants