-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
libssh2: also try fopen()
when looking for implicit keys
#13571
Conversation
Replace `stat()` calls with `fopen(.., FOPEN_READTEXT)`, which is how libssh2 opens these files at a later point. Follow-up to 602fc21 curl#13498 Bug: curl@602fc21#r141676880 Reported-by: Harry Sintonen Closes #xxxxx
If I can comment on this, using |
The issue this is trying to address is that the check was using a libssh2 already opens the files with Doing To avoid doing double |
@vszakats Thank you |
stat()
with fopen()
for existence checksfopen()
when looking for implicit keys
{ | ||
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
filename
checked
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Talking about looking up implicit keys on the disk (when there was none
I'd be happy to stay with 1., but wouldn't go there without a second opinion. |
Judging by the silence there seems to be no good solution to this and/or no interest, so I'm closing this proposal. IMO from a libcurl security angle this issue would be best served by not looking up keys implicitly on the disk and require an explicit key. If searching for implicit keys is desired, it would be better done in the curl tool. With libssh this may need more research, because it searches for keys on its own. A next step would be loading such keys in memory in one step, as part of the key search. |
Replace
stat()
calls withfopen(.., FOPEN_READTEXT)
, which ishow libssh2 opens these files at a later point.
Follow-up to 602fc21 #13498
Bug: 602fc21#r141676880
Reported-by: Harry Sintonen
Closes #13571
/cc @piru