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

Omitting invalid timestamps (v3) #14

Closed
jscheid opened this issue Jun 23, 2022 · 1 comment
Closed

Omitting invalid timestamps (v3) #14

jscheid opened this issue Jun 23, 2022 · 1 comment
Assignees
Labels
Milestone

Comments

@jscheid
Copy link

jscheid commented Jun 23, 2022

sftpserver/v3.c

Lines 115 to 123 in 62913a9

/* Check that the conversion was sound. SFTP v3 becomes unsound in 2038CE.
* If you're looking at this code then, I suggest using a later protocol
* version. If that's not acceptable, and you either don't care about
* bogus timestamps or have some other workaround, then delete the
* checks. */
if(m != attrs->mtime.seconds)
sftp_fatal("sending out-of-range mtime");
if(a != attrs->atime.seconds)
sftp_fatal("sending out-of-range mtime");

The comment here is a bit misleading when it says that V3 will become unsound in 2038, since V3 also can't represent timestamps before 1970 and indeed the condition here triggers in this case. It's easy to imagine legitimate uses for such old timestamps, and of course that's why V5 switched to signed values.

Also I've come across a few files from the far future, usually resulting from unpacking a corrupted archive.

At any rate, using sftp_fatal here is a little unfortunate. First, despite the name and its documentation, it doesn't actually terminate the process so the connection just hangs. But even if it did close the connection, this would be a pretty harsh treatment when all the client wanted to do was learn the attributes of a file. Would it be better to simply omit the timestamps from the output in this case? A white lie, so to speak?

@ewxrjk
Copy link
Owner

ewxrjk commented Jul 10, 2022

Agreed that the behaviour is harsh, and should substitute some other value.

However I'm confused by the statement that sftp_fatal does not terminate the process. It calls either exit or _exit both of which end the process, and when I tested with a far-future timestamp the process did terminate.

[pid 251222] write(2, "FATAL: ", 7)     = 7
[pid 251222] write(2, "sending out-of-range mtime", 26) = 26
[pid 251222] write(2, "\n", 1)          = 1
[pid 251222] exit_group(-1 <unfinished ...>
[pid 251225] <... futex resumed>)       = ?
[pid 251224] <... futex resumed>)       = ?
[pid 251223] <... futex resumed>)       = ?
[pid 251222] <... exit_group resumed>)  = ?
[pid 251221] <... read resumed> <unfinished ...>) = ?
[pid 251225] +++ exited with 255 +++
[pid 251224] +++ exited with 255 +++
[pid 251222] +++ exited with 255 +++
[pid 251223] +++ exited with 255 +++
+++ exited with 255 +++

@ewxrjk ewxrjk self-assigned this Jul 10, 2022
@ewxrjk ewxrjk added the bug label Jul 10, 2022
@ewxrjk ewxrjk added this to the Release 3 milestone Jul 10, 2022
@ewxrjk ewxrjk closed this as completed in cca7888 Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants