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

[vszakats]

Replace stat() calls with fopen(.., FOPEN_READTEXT), which is
how libssh2 opens these files at a later point.

Follow-up to 602fc21 #13498

Bug: 602fc21#r141676880
Reported-by: Harry Sintonen
Closes #13571

---

/cc @piru

[rsbeckerca]

If I can comment on this, using fopen() instead of stat() has non-portable side-effects. fopen() modifies attributes in the file system (st_atime) and is a heavy-weight operation on some platforms. stat() is a directory lookup that can be much faster.

[vszakats]

The issue this is trying to address is that the check was using a
different method than the actual use, which may lead to issues.

libssh2 already opens the files with fopen() so doesn't that mean
that its side-effects were there already? I can't see how doing it
twice would make those worse, except in perfmance.

Doing stat() and fopen() is an option, and it might make
stepping over non-existing files faster. Would that be useful?

To avoid doing double fopen(), it'd theoretically be possible to load
the key right away on the first successful open, but I couldn't figure
out how to do it (libssh2 doesn't have a private key pre-load API). At
best it seemed to need a refactor for the key loading logic, which I'm
not prepared to do.

[rsbeckerca]

@vszakats Thank you

[vszakats]

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

Any suggestion how to deal with this?

[vszakats]

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

[vszakats]

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.

[vszakats]

Talking about looking up implicit keys on the disk (when there was none
specified explicitly), what would be the best way to tackle it from
a security / performance angle?:

1. stat() + fopen() → fast but CodeQL High severity warning.
2. stat() → fast but result not the same as fopen(), as noted by @piru.
3. fopen() → might be slow, as noted by @rsbeckerca.
4. access() → portability issues, banned from curl.
5. something else?

I'd be happy to stay with 1., but wouldn't go there without a second opinion.
In case of doubt I believe we might swallow the perf drop and go with 2.

[vszakats]

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.