Vulnerabilities in FileUplink overwriting

[rrieber]

F´ Version
FileUplink

---

Problem Description

CADRE discovered a vulnerability with FileUplink associated with overwriting existing files. Assume version-1 of a file is saved with the name and has the size X-bytes. Version-2 of is uplinked with a size of Y-bytes where X>Y, e.g. the new file is smaller than the old file. In this scenario, FileUplink successfully validates size, checksum, etc, then saves to disk in such a way that it partially overwrites the old version. The result is that has a size of X-bytes. The first Y-bytes originate in FILE.bin_v2 and the remaining (X minus Y)-bytes originate from FILE.bin_v1. And what is concerning is that no errors or warnings are raised.

CADRE is taking this as Use-As-Is (UAI) and operationally restricting file overwrites on uplink.

For JPLers, reference CADRE issue 946.

According to @csmith608, other FPrime missions at JPL have addressed this problem in their local code base, so there are solutions out in the wild.

How to Reproduce

See above

Expected Behavior

The old file deleted prior to the new file being saved.

[Joshua-Anderson]

A bit of context on why the existing behavior is desired by some projects:

FileUplink intended to be used for receiving file downlinks over unreliable links (i.e. radio). Because of this, the expectation is that files will need to be retransmitted, or transmitted partially over multiple activities.

Example: Send a 3 MB files in 3 x 1 MB chunks, then later retransmit a 50 KB chunk in the middle of the file.

The goal was for FileUplink to be safe for data downlink - in that it would never destroy data, only overwrite.

[Joshua-Anderson]

The problem as I view it is that there's two different use cases for FileUplink:

• Receiving critical files sent TO a deployment
• Receiving critical data sent FROM a deployment.

And you want different behaviors for both cases.

When receiving data from a deployment, you never want to destroy data, since you may never have another chance to transmit that data.

When sending files to a deployment, file integrity is paramount, since corrupted commands/configuration can cause irrecoverable damage.

[Joshua-Anderson]

In the shorter term, I think it's probably worth adding a easy to use file checksum command to F' (maybe in FileManager?) to at least make it easy to validate file integrity if this footgun occurs.

For what it's worth, the projects I've worked on have had minimal issues with this limitation, since there is a strong preference to version rather than overwrite files (I.E. importantfile.1, importantfile.2), though that's not always possible.

In the longer term, it's probably worth making FileUplink more configurable by allowing folks to chose between data loss and file integrity protections.

[csmith608]

The problem as I view it is that there's two different use cases for FileUplink:

* Receiving critical files sent TO a deployment

* Receiving critical data sent FROM a deployment.

And you want different behaviors for both cases.

When receiving data from a deployment, you never want to destroy data, since you may never have another chance to transmit that data.

When sending files to a deployment, file integrity is paramount, since corrupted commands/configuration can cause irrecoverable damage.

I don't really understand your point here, wouldn't file uplink only effect receiving critical files sent TO a deployment?

[Joshua-Anderson]

I don't really understand your point here, wouldn't file uplink only effect receiving critical files sent TO a deployment?

FileUplink is also used for sending files between F' deployments

[csmith608]

So, I think I see that this was an intended behavior, but I still think that this is an intricacy that is not well documented or communicated through FSW. I'd like to solve this and it could be something like modifying the successful uplink EVR, outputting an EVR when the file close comes at a file size smaller than the current file (Rob's suggestion), add a different or another CRC check, etc

[bocchino]

Maybe I'm missing something, but it seems this revised algorithm should handle all the comments and use cases so far:

1. Open the file
2. For each data packet received, write it into the file
3. When receiving an END packet, check the CRC
4. If the CRC matches, trim the file to the actual size of data received
5. Close the file

I think the current algorithm does 1-3 and then 5. I think we could just add 4 to fix the bug (i.e., complete file received, but file on disk has leftover data from a previous file with the same name). I don't see any need for special case handling here.

[bocchino]

Note: I'm assuming that we currently check the CRC against the file data. To do that, we have to know the size of the data in the file, so we should be able to trim the physical file to the size of the data. I think we're just skipping that step.

[bocchino]

Also, looking at the design, it seems that the CRC in the end packet is compared against the CRC computed from the file data. So I don't understand how the CRC can be validated in the case that there is extra leftover data in the file. It seems like the extra data would be included in the CRC computation, invalidating the CRC.

I think a robust solution would be (1) include the file size S in the end packet; (2) when checking the CRC, check the first S bytes of the file; (3) if the CRC matches, then trim the file to size S.

[bocchino]

Note that this solution supports retransmission of packets. For example, suppose there are 10 data packets, 9 are received and 1 is dropped. On the next transmission, you can send the 1 data packet and the end packet with the file size and the CRC. Once all the packets are in, the CRC should match. At that point you can trim the file to the expected size and declare success.

[timcanham]

Does FileDownlink or the ground system support the partial retransmit with the full file CRC? FileDownlink at least should support this since as @Joshua-Anderson mentioned, it is used to transmit files between deployments.

[Joshua-Anderson]

Also, looking at the design, it seems that the CRC in the end packet is compared against the CRC computed from the file data.

The FileUplink CRC is calculated from received packets (the checksum is updated on each received packet) not file contents. So FileUplink is currently unaware there's preexisting data remaining at the end of the file.

Does FileDownlink or the ground system support the partial retransmit with the full file CRC?

FileUplink currently works by calculating the CRC of the portion of the file that is transferred. I'd be somewhat concerned that sending the checksum of the entire file could interact poorly with sending a file in multiple chunks.

To do that, we have to know the size of the data in the file, so we should be able to trim the physical file to the size of the data

I'm a little concerned with allowing FileUplink to "delete" data by trimming files. Getting data to spacecraft can be very expensive, so we should be very careful about allowing unexpected data deletion

[LeStarch]

The CCB is recommending these steps:

1. Send size in start packet and if file exists, truncate to that size if larger.
2. Add parameter for full-file CRC
3. Add full-file CRC to end packet
4. Flag for when not including CRC

[Joshua-Anderson]

Add parameter for full-file CRC

Instead of adding a full file CRC, why not add a CRC32 file command to file manager?

In my view, FileUplink is responsible for ensuring the integrity of the received file data/packets, but integrity of files in the filesystem itself is a more general responsibility that potentially belongs elsewhere.

[Joshua-Anderson]

Send size in start packet and if file exists, truncate to that size if larger.

This seems like it could get awkward when transferring files of unknown size (I.E. in a scenario of transferring from one deployment to another)

I might instead recommend just adding a truncate flag, which will truncate anything past the end of the current file transfer. This works as expected for both single chunk file uplinks, and file uplinks of subsequent chunks of the same file. In the rare case you are uplinking to the middle of an existing file, truncate should be set to false.

A true/false flag will also take fewer bytes than a total file size flag.

[bocchino]

This seems like it could get awkward when transferring files of unknown size (I.E. in a scenario of transferring from one deployment to another)

Can't you get the file size from the file system in this case?

I might instead recommend just adding a truncate flag, which will truncate anything past the end of the current file transfer.

This seems to introduce a danger of truncating data you don't want to truncate (i.e., send missing packets and forget to set truncate=false).

[Joshua-Anderson]

If you're using command sequences you must populate the expected file size at sequencing time. While ideally a file listing is performed to get a file size, there are circumstance where's not available at sequencing time.

This seems to introduce a danger of truncating data you don't want to truncate (i.e., send missing packets and forget to set truncate=false).

Truncation for complete (non-partial) file uplinks is always safe. Partial file uplinks are a minority case, and partial uplinks in the middle of a file are a subset of that subset. Given how rare retransmitting a partial uplink in the middle of a file is I think that's a less-likey risk than an operator accidentally providing an incorrect filesize and unintentionally truncating a file.