ModifiableFileWatcher should watch for changes in BasicFileAttributes.fileKey() #370
Comments
|
It probably should (at least if a file key is available). IIRC, on Windows there are no file keys. Not sure about other file systems. |
Agreed. Javadoc for
So the code just needs to treat "null" as a value like any other, and then everything will work - file keys will be used if available, otherwise if the platform doesn't support file keys then we will fallback gracefully to the existing logic. |
|
Still not foolproof, though. There is still the possibility of the file appearing "racily clean" (basically, having been modified again, with content of equal size, but faster than the file timestamp resolution). If one wants to catch such cases, too, then we'd have to apply concepts used in JGit: FileSnapshot, and considering any "last modified" time with now - last_modified <= file_timestamp_resolution a potential modification. Probably we wouldn't need the machinery JGit uses to determine the file timestamp resolution accurately since we don't read these files that frequently, so just using a worst-case assumption of 3 seconds might be fine? |
Just to be clear, I wasn't claiming this would be foolproof - just relatively more foolproof than before. I think any truly foolproof scheme would require reading the file anew every time (I think this is also what you're saying - not sure because I'm unfamiliar with JGit). Of course you wouldn't have to actually parse it every time - instead, you'd first check whether the contents hadn't changed (e.g., compute secure hash and compare to previous), and if not then re-use the existing credentials. This type of check would be relatively quick. |
Additionally, handle the case of files being modified very quickly, such that the last modified timestamp doesn't change, even though the file was modified. If the modification did not change the file size, such metadata is "racily clean", meaning it indicates the file had not been modified when in fact it was. This can occur because of the finite resolution of file timestamps. If the file timestamp has a granularity of 2 seconds (FAT), two file modifications within these two seconds that don't change the file size cannot be recognized. Hence any file timestamp from the past 2 seconds (measured from the time it was read) is suspect, and the file must be considered potentially modified, and must be re-loaded. This is not a problem unless one frequently writes files and then reads them again within two seconds. (Or if one reads the same file multiple times within two seconds.) In such cases, care should be taken to determine the actual resolution of file timestamps, which often is much better. But for the use cases in SSH the worst-case assumption of two seconds should be fine. Bug: apache#370
Additionally, handle the case of files being modified very quickly, such that the last modified timestamp doesn't change, even though the file was modified. If the modification did not change the file size, such metadata is "racily clean", meaning it indicates the file had not been modified when in fact it was. This can occur because of the finite resolution of file timestamps. If the file timestamp has a granularity of 2 seconds (FAT), two file modifications within these two seconds that don't change the file size cannot be recognized. Hence any file timestamp from the past 2 seconds (measured from the time it was read) is suspect, and the file must be considered potentially modified, and must be re-loaded. This is not a problem unless one frequently writes files and then reads them again within two seconds. (Or if one reads the same file multiple times within two seconds.) In such cases, care should be taken to determine the actual resolution of file timestamps, which often is much better. But for the use cases in SSH the worst-case assumption of two seconds should be fine. Bug: apache#370
|
Awesome, thanks. |
Version
sshd-2.9.2-55-gd4b951a10
Bug description
ModifiableFileWatcheris supposed to detect when a file changes.However, it's possible for a file to change and for
ModifiableFileWatcherto not detect the change.Consider the following somewhat contrived but not unreasonable scenario:
/opt/pubkey/adam,/opt/pubkey/jeff, etc./var/pubkey/admin -> /opt/pubkey/adamnew AuthorizedKeysAuthenticator(new File("/var/pubkey/admin").toPath())Adam gets fired for being an evil hacker, so we quickly make Jeff the new admin like this:
Phew! We think Adam is no longer authorized, but...
/var/pubkey/adminhas never not existed at any point in time/opt/pubkey/adamand/opt/pubkey/jeffare the exact same size since they contain the same type of public key and the usernames (added as a comment after the public key) are the same length/opt/pubkey/adamand/opt/pubkey/jeffhave the same modification timestamp, since they were installed at the same time as part of the same RPMTherefore,
ModifiableFileWatcherwill fail to notice that the file has changed. So evil Adam is still the admin!Solution: Along with
lastExisted,lastModifed, andlastSize,ModifiableFileWatchershould also track the value ofBasicFileAttributes.fileKey()in a fieldlastFileKey, and detect a file change if this value changes.Actual behavior
No file change detected.
Expected behavior
File change detected.
Relevant log output
No response
Other information
No response
The text was updated successfully, but these errors were encountered: