broken sftp_quote logic #2143

Closed
bagder opened this Issue Dec 2, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@bagder
Member

bagder commented Dec 2, 2017

in the beginning of the function lib/ssh-libssh.c:sftp_quote() there's this line:

char *cmd = sshc->quote_item->data;

This is fine, because we know sshc->quote_item to be non-NULL due to earlier checks before this function is called.

This cmd pointer is then dereferenced and possibly increased but on line 2528 it is checked for NULL?

if(cmd) {

... if cmd would be NULL there, it would already have done wrong and crashed. It can't be NULL there!

But the if condition always ends with a return; and below, outside of the if block (on line 2657 in my version - this means this is code that can never be reached: dead code), there's a final check if sshc->quote_item is NULL (which again, it can't be) and if it is, it would call state()...

(CID 1424902 by coverity, interpreted a bit by me)

/cc @nmav

@bagder bagder added the SCP/SFTP label Dec 2, 2017

@nmav

This comment has been minimized.

Show comment Hide comment
@nmav

nmav Dec 3, 2017

Contributor

That function code is very similar with the SSH_SFTP_QUOTE: block in the ssh.c state machine. The same behavior seems to be there too. The if null check seems indeed incorrect. However, whether cmd is null at the call of this function, I cannot really tell. It depends on whether a quote_item can contain null data

Contributor

nmav commented Dec 3, 2017

That function code is very similar with the SSH_SFTP_QUOTE: block in the ssh.c state machine. The same behavior seems to be there too. The if null check seems indeed incorrect. However, whether cmd is null at the call of this function, I cannot really tell. It depends on whether a quote_item can contain null data

@nmav

This comment has been minimized.

Show comment Hide comment
@nmav

nmav Dec 3, 2017

Contributor

I think the coverity errors 1424908, 1424902 and 1424901 are also shared between the two back-ends, though the last looks like a false negative. That is pretty much an argument for @kdudka 's suggestion to make the SCP/SFTP part separated from the back-end.

Contributor

nmav commented Dec 3, 2017

I think the coverity errors 1424908, 1424902 and 1424901 are also shared between the two back-ends, though the last looks like a false negative. That is pretty much an argument for @kdudka 's suggestion to make the SCP/SFTP part separated from the back-end.

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Dec 4, 2017

Member

However, whether cmd is null at the call of this function, I cannot really tell

But if it could, it still would be pointless to check that after it has already been dereferenced / increased!

And thanks for pointing out that this flaw exists in lib/ssh.c too. I'll take care of that in a PR!

Member

bagder commented Dec 4, 2017

However, whether cmd is null at the call of this function, I cannot really tell

But if it could, it still would be pointless to check that after it has already been dereferenced / increased!

And thanks for pointing out that this flaw exists in lib/ssh.c too. I'll take care of that in a PR!

bagder added a commit that referenced this issue Dec 4, 2017

libssh2: remove dead code from SSH_SFTP_QUOTE
Figured out while reviwring code in the libssh backend. The pointer was
checked for NULL after having been dereferenced, so we know it would
always equal true or it would've crashed.

Pointed-out-by: Nikos Mavrogiannopoulos

Bug #2143

bagder added a commit that referenced this issue Dec 4, 2017

libssh2: remove dead code from SSH_SFTP_QUOTE
Figured out while reviewing code in the libssh backend. The pointer was
checked for NULL after having been dereferenced, so we know it would
always equal true or it would've crashed.

Pointed-out-by: Nikos Mavrogiannopoulos

Bug #2143
Closes #2148

bagder added a commit that referenced this issue Dec 5, 2017

libssh: remove dead code in sftp_qoute
... by removing a superfluous NULL pointer check that also confuses
Coverity.

Fixes #2143

@bagder bagder closed this in 85f0133 Dec 5, 2017

JohnDeHelian pushed a commit to JohnDeHelian/curl that referenced this issue Dec 7, 2017

libssh: remove dead code in sftp_qoute
... by removing a superfluous NULL pointer check that also confuses
Coverity.

Fixes #2143
Closes #2153

@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.