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.

Sign up for GitHub

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

Jump to bottom

libssh2: also try fopen() when looking for implicit keys #13571

Closed
wants to merge 3 commits into from
Closed

libssh2: also try fopen() when looking for implicit keys #13571

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
150ca41
libssh2: replace `stat()` with `fopen()` for existence checks
vszakats May 9, 2024
640e09b
readd stat() to skip over non-existing files faster
vszakats May 9, 2024
534cca0
rename check function, return bool
vszakats May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 19 additions & 5 deletions lib/vssh/libssh2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,19 @@
return result;
}

static int libssh2_key_exists(const char *filename)
{
struct_stat sbuf;
if(!stat(filename, &sbuf)) {
FILE *fd = fopen(filename, FOPEN_READTEXT);

Check failure

Code scanning / CodeQL

Time-of-check time-of-use filesystem race condition High

The
filename
being operated upon was previously
checked
, but the underlying file may have been changed since then.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically true, but the bigger issue is the double fopen(), not this. IMO.

Any suggestion how to deal with this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems OpenSSH does a stat(), then opens the file. Same this code did before this PR.

https://github.com/openssh/openssh-portable/blob/cbbbf76aa6cd54fce32eacce1300e7abcf9461d4/ssh-add.c#L1032

Copy link
Member Author

@vszakats vszakats May 10, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options are to ignore this warning, or to delete stat() to make it go away and pay the extra runtime.

I have a patch pending that will add 2 * 2 more id_* files to check, to match libssh.
Also worth noting that these checks are only performed if the user did not pass a private
key to (lib)curl explicitly via --key or CURLOPT_SSH_PRIVATE_KEYFILE. It also
doesn't seem very safe to rely on the implicit keys, because anybody with FS or env
access can override them trivially.

if(fd) {
fclose(fd);
return 1;
}
}
return 0;
}

/*
* ssh_statemach_act() runs the SSH state machine as far as it can without
* blocking and without reaching the end. The data the pointer 'block' points
Expand Down Expand Up @@ -1086,20 +1099,20 @@
/* To ponder about: should really the lib be messing about with the
HOME environment variable etc? */
char *home = curl_getenv("HOME");
struct_stat sbuf;

/* If no private key file is specified, try some common paths. */
if(home) {
/* Try ~/.ssh first. */
sshc->rsa = aprintf("%s/.ssh/id_rsa", home);
if(!sshc->rsa)
out_of_memory = TRUE;
else if(stat(sshc->rsa, &sbuf)) {
else if(!libssh2_key_exists(sshc->rsa)) {
Curl_safefree(sshc->rsa);

sshc->rsa = aprintf("%s/.ssh/id_dsa", home);
if(!sshc->rsa)
out_of_memory = TRUE;
else if(stat(sshc->rsa, &sbuf)) {
else if(!libssh2_key_exists(sshc->rsa)) {
Curl_safefree(sshc->rsa);
}
}
Expand All @@ -1108,10 +1121,11 @@
if(!out_of_memory && !sshc->rsa) {
/* Nothing found; try the current dir. */
sshc->rsa = strdup("id_rsa");
if(sshc->rsa && stat(sshc->rsa, &sbuf)) {
if(sshc->rsa && !libssh2_key_exists(sshc->rsa)) {
Curl_safefree(sshc->rsa);

sshc->rsa = strdup("id_dsa");
if(sshc->rsa && stat(sshc->rsa, &sbuf)) {
if(sshc->rsa && !libssh2_key_exists(sshc->rsa)) {
Curl_safefree(sshc->rsa);
/* Out of guesses. Set to the empty string to avoid
* surprising info messages. */
Expand Down