View Issue Details

IDProjectCategoryView StatusLast Update
0006149GNUnetutil librarypublic2021-09-02 18:23
Reporterfefe Assigned ToChristian Grothoff  
PrioritynormalSeveritymajorReproducibilityN/A
Status closedResolutionfixed 
Product VersionGit master 
Target Version0.13.0Fixed in Version0.13.0 
Summary0006149: (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 ReproduceHere 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.
TagsNo tags attached.

Activities

Christian Grothoff

2020-04-02 19:11

manager   ~0015492

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.

Florian Dold

2020-04-03 10:03

developer   ~0015495

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.

Christian Grothoff

2020-04-03 10:48

manager   ~0015497

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).

Christian Grothoff

2020-04-11 22:01

manager   ~0015626

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.

schanzen

2020-07-09 09:17

administrator   ~0016425

0.13.0 released

Christian Grothoff

2021-09-02 18:23

manager   ~0018381

Fix committed to master branch.

Related Changesets

merchant: master 3b341f88

2020-04-11 23:46

Christian Grothoff


Details Diff
adapting to API changes for fix 0006149 Affected Issues
0006149
mod - src/backend/taler-merchant-httpd.c Diff File

Issue History

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 Adminknox 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