premature fopen in entropy_write_seed_file()

[gavacq]

Description

• Type: Bug
• Priority: Major

---

Bug

mbedtls_entropy_write_seed_file() opens the seed file before calling entropy_func() to grab some entropy.

The issue occurs when the user has added a seed file poll function as a custom entropy poll source. When gathering entropy from available sources, fopen() will then be called again on an already open file. This can result in file corruption.

This could be fixed by moving fopen() after the call to entropy func() in mbedtls_entropy_write_seed_file().

[militant-daos]

Hi @geecrypt , I'd like to work on this issue but I do not see any problems with mbedtls_entropy_write_seed_file() logic itself. To be precise, taking into account all the supplied tests I cannot consider a straightforward way to open a seed file more than once during mbedtls_entropy_write_seed_file() call. Could you please elaborate bit on what should be done to trigger this behavior?

The only possible case for the file to be corrupted IMHO if one of our entropy sources opens the same file to read from it while it being already opened by mbedtls_entropy_write_seed_file but I doubt this is a valid use-case which MbedTLS should handle by itself (I believe that the users of the library are responsible for entropy sources they implement in terms of behavior correctness). Please correct me if I'm wrong. Thanks in advance.

In general I agree with you about moving fopen() call below mbedtls_entropy_func() besides the described problem with corruption: since in case if mbedtls_entropy_func fails there is no sense in opening an output file at all - we may skip this action saving some time not doing the syscall.

@gilles-peskine-arm CC. This is my first attempt to contribute to this project so I may ask some stupid questions :-)

[gilles-peskine-arm]

I think I see the problem:

1. The application calls mbedtls_entropy_write_seed_file.
2. mbedtls_entropy_write_seed_file opens the file for writing and truncated it (fopen call).
3. mbedtls_entropy_write_seed_file calls mbedtls_entropy_func.
4. It's the initial entropy run so mbedtls_entropy_func calls mbedtls_entropy_update_nv_seed.
5. mbedtls_entropy_update_nv_seed calls mbedtls_nv_seed_write.
6. mbedtls_nv_seed_write writes to the same seed file.

This may be a user error, though. Does it really make sense to call mbedtls_entropy_write_seed_file on the entropy file that Mbed TLS would update anyway?

We can't portably detect whether two files are the same, so it isn't a condition that we can detect at runtime. We can only document that you shouldn't do this.

@geecrypt Is there a problem if mbedtls_entropy_write_seed_file is called on a file which is not the seed file? The entropy code is sufficiently complex that I can believe that there's another problematic code path. (It's on our list of things to rewrite in Mbed TLS 3.0.)

@militant-daos Feel free to ask, either on GitHub or on the mailing list. I'm aware that there are many things we don't document. We're working on it.

[militant-daos]

@gilles-peskine-arm thanks for the explanation. Yes, you're right - I did not define MBEDTLS_ENTROPY_NV_SEED before checking and thus did not follow the whole execution path, down to the platform-specific code when I examined the code first time.

Looks like the only way to break things is to make path arg given to mbedtls_entropy_write_seed_file to be equal to MBEDTLS_PLATFORM_STD_NV_SEED_FILE or vice versa which does not look like a valid use-case to me, however I may be wrong - it looks like nothing stops a user from doing that. I did not spot any other issues related to file access API.

I have created a PR for this - #3616 . In case if you consider this issue as the one which does not need to be fixed please fell free to abandon it and I'll look for another one to work on.

[yanesca]

I agree, it is not a supported use case, but there is nothing stopping users to do that.

When the NV_SEED feature was added, they complemented each other with the *_seed_file() functions, but as the library evolved and the platform module was extended, there is little benefit of having the latter and we could consider deprecating/removing them.

That said, it is hard to say if or when that will happen and given that we have a fix available (thank you @militant-daos!), I suggest we fix this.

[gavacq]

@gilles-peskine-arm Another example:

1. User adds mbedtls_nv_seed_poll to entropy sources
2. User wants to update the NV seed file at some time interval to store gathered entropy, so they call mbedtls_entropy_update_seed_file
3. The mbedtls_entropy_write_seed_file logic proceeds as you described
4. While its not the initial run, entropy_func runs through all sources to gather entropy and hits mbedtls_nv_seed_poll.
5. mbedtls_nv_seed_poll opens and closes the seed file in mbedtls_platform_std_nv_seed_read. I think this was causing the file issues I experienced.