View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006149 | GNUnet | util library | public | 2020-04-02 16:21 | 2021-09-02 18:23 |
| Reporter | fefe | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | major | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | Git master | ||||
| Target Version | 0.13.0 | Fixed in Version | 0.13.0 | ||
| Summary | 0006149: (really a gnunet problem) GNUNET_CRYPTO_eddsa_key_create_from_file is a really bad API | ||||
| Description | 72 /** 73 * Create a new private key by reading it from a file. If the 74 * files does not exist, create a new key and write it to the 75 * file. Caller must free return value. Note that this function 76 * can not guarantee that another process might not be trying 77 * the same operation on the same file at the same time. 78 * If the contents of the file 79 * are invalid the old file is deleted and a fresh key is 80 * created. From an ops perspective this means that the exchange needs write access to the key. This is not good. From a reliability perspective it means this code must deal with several potential race conditions, which makes it very complicated and potentially dangerous. | ||||
| Steps To Reproduce | Here is some code from the function: 96 if (GNUNET_SYSERR == GNUNET_DISK_directory_create_for_file (filename)) 97 return NULL; and GNUNET_DISK_directory_create_for_file does this: 697 if (0 == access (rdir, W_OK)) 698 { 699 GNUNET_free (rdir); 700 return GNUNET_OK; 701 } This breaks if the key directory is read-only (which it should be!). It is never a good idea to use access(2) in the first place. Use open(2) directly. It will tell you all you need to know. Also note that the code uses GNUNET_DISK_OPEN_FAILIFEXISTS which relies on O_EXCL. O_EXCL can fail on NFS below version 3. Generally the proper solution is to write the data to a new file under a new, unique name, and then use rename(2) to atomically move it over to the proper name when all has been written to it. Or, if you don't want to overwrite the existing key file, use link(2) and then unlink the temp file. The locking may be unreliable over NFS and/or open an additional denial of service attack vector if someone locks the file and then never unlocks it. In newer kernels you can also use renameat2(2) with the RENAME_NOREPLACE flag. Because all this stuff is error prone and complicated, the general recommendation is to not do it automatically and implicitly but move key generation to its own little helper program. Then make sure that the key is owned by root and not writable for the user the exchange service is running as. Also note that the code currently does not even try to worry about symlink attacks. | ||||
| Tags | No tags attached. | ||||
|
|
If the key directory were (always) read-only, we could not deploy new keys, which we must because of key rotation. Note that the exchange automatically DELETES private keys when the respective usage periods expire. So the directory MUST be writable for the exchange. I also don't see how the exchange being (theoretically) able to change its keys would be a security problem. Key disclosure is a problem, modification/deletion is at best an availability issue. However, at the point where the attacker can cause that, I guess we'd hope that they merely delete or modify the keys ;-). I also don't see any advantage of the files being owned by root, that would imply that we'd need a SUID helper to read it, which seems worse -- starting with deployment complexity -- for no clear benefit. As for the race condition, in a proper deployment there is only one process (taler-exchange-keyup or admin via 'cp/mv') under manual control setting up such fresh keys, so that's again not guarded against because it is not a problem. The keys are in a directory owned by the exchange user, so someone exploiting a file race in that directory is not at all the same as race conditions on say /tmp where other users (very theoretically) may have access. Ditto for NFS: we assume sane deployments do NOT store private keys on NFS, just as we also assume that sane deployments turn off swap. In summary, on this one I'm not sure your cure is applicable and/or not worse than the problem. |
|
|
First of all, I agree that GNUNET_CRYPTO_eddsa_key_create_from_file is a bad API, and I've argued for that point before. The two bad aspects I agree with are: 1. Non-atomicity of writing key files. Yes, you can argue that this is not a problem in practice, or not a problem in our (i.e. "Taler System SA") deployment. But it's a GNU project, other people may want to run it under different circumstances, and we should just make key generation atomic! 2. IMHO we *must* separate generating the keys from reading the keys. The current approach is not defensible, and it has lead to confusing problems in our existing demo deployment already, where a new key was created because the existing one wasn't in the right place and this lead to issues later on. We already *have* a helper program to do key generation, and I think it makes sense to never have the exchange HTTPD process create any keys. This wouldn't even be a big change. But then, I don't understand Fefe's suggestion of using root (why root?) to generate and own private key material. You could argue that an attacker that can make the taler-exchange-httpd process delete key files can cause rather severe availability issues, as to generate new signing keys and denomination keys, we need to use the exchange's offline master key and ask the auditor to approve new denomination keys. That takes time and manual intervention. But if an attacker managed to get the exchange HTTPD to delete these keys, we MUST assume that they were also able to read the key material. In that case, we must revoke all online keys *anyway*. So in summary, I don't quite see what security or availability aspect would be improved by the privilege separation. |
|
|
I think the privilege separation is another story entirely, and Felix is confusing things here because of his misguided obsession with the write permission on the directory. What will eventually make sense is to have another user (say 'exchange-keys') that truly has the read-access to to the key material (file or HSM), and we fork-exec some SUID binary that is 750 exchange-keys:exchange owned from the taler-exchange-httpd and use IPC with that child to do our signatures. That way, the "big" exchange-httpd process never has the private keys in its RAM, and the (simpler) child process can do the signing/HSM-interaction and counting of the # signatures and *nothing* else, providing an additional layer of isolation for the key material. I've considered this so far a 1.x feature, especially given that we'd need to get an HSM supporting blind signatures, which according to my research is non-trivial as the PKCS-standard doesn't include blind signatures yet. I don't fundamentally disagree with Florian's (1)+(2) -- just didn't have priority (so far). |
|
|
Modified the GNUnet API to 1) use the 'link() + unlink()' method to ensure atomicity 2) take an argument whether key creation or merely key reading is desired 3) when creating a file, use fchmod() to set it to read-only (400) before proceeding with the write() Modified Taler exchange to: 1) Use new API 2) refuse to do key creation by accident in taler-auditor-httpd, keys are now only generated in the (existing) helpers taler-auditor-sign and the exchange-tools key-helper.c routines. Equivalent modifications were made for instance keys in the merchant backend. Note: this does NOT implement privilege separation in the Taler exchange as that is a separate, Taler-specific issue. |
|
|
0.13.0 released |
|
|
Fix committed to master branch. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-04-02 16:21 | fefe | New Issue | |
| 2020-04-02 16:21 | fefe | Status | new => assigned |
| 2020-04-02 16:21 | fefe | Assigned To | => Christian Grothoff |
| 2020-04-02 19:11 | Christian Grothoff | Note Added: 0015492 | |
| 2020-04-03 10:03 | Florian Dold | Note Added: 0015495 | |
| 2020-04-03 10:48 | Christian Grothoff | Note Added: 0015497 | |
| 2020-04-03 14:25 | Christian Grothoff | Project | Taler => GNUnet |
| 2020-04-03 14:25 | Christian Grothoff | Category | exchange => General |
| 2020-04-03 18:55 | Christian Grothoff | Category | General => util library |
| 2020-04-03 18:55 | Christian Grothoff | Product Version | 0.7.0 => Git master |
| 2020-04-11 22:01 | Christian Grothoff | Status | assigned => resolved |
| 2020-04-11 22:01 | Christian Grothoff | Resolution | open => fixed |
| 2020-04-11 22:01 | Christian Grothoff | Fixed in Version | => 0.12.2 |
| 2020-04-11 22:01 | Christian Grothoff | Note Added: 0015626 | |
| 2020-04-11 22:02 | Christian Grothoff | Target Version | => 0.12.2 |
| 2020-04-23 10:45 | schanzen | Fixed in Version | 0.12.2 => 0.13.0 |
| 2020-04-23 10:47 | schanzen | Target Version | 0.12.2 => 0.13.0 |
| 2020-06-01 00:52 |
|
Issue cloned: 0006354 | |
| 2020-07-09 09:17 | schanzen | Note Added: 0016425 | |
| 2020-07-09 09:17 | schanzen | Status | resolved => closed |
| 2021-09-02 18:22 | Christian Grothoff | Changeset attached | => Taler-merchant master 3b341f88 |
| 2021-09-02 18:23 | Christian Grothoff | Note Added: 0018381 |
