summaryrefslogtreecommitdiff
path: root/gpg-interface.c
AgeCommit message (Collapse)AuthorFilesLines
2020-07-28strvec: fix indentation in renamed callsLibravatar Jeff King1-7/+7
Code which split an argv_array call across multiple lines, like: argv_array_pushl(&args, "one argument", "another argument", "and more", NULL); was recently mechanically renamed to use strvec, which results in mis-matched indentation like: strvec_pushl(&args, "one argument", "another argument", "and more", NULL); Let's fix these up to align the arguments with the opening paren. I did this manually by sifting through the results of: git jump grep 'strvec_.*,$' and liberally applying my editor's auto-format. Most of the changes are of the form shown above, though I also normalized a few that had originally used a single-tab indentation (rather than our usual style of aligning with the open paren). I also rewrapped a couple of obvious cases (e.g., where previously too-long lines became short enough to fit on one), but I wasn't aggressive about it. In cases broken to three or more lines, the grouping of arguments is sometimes meaningful, and it wasn't worth my time or reviewer time to ponder each case individually. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-07-28strvec: convert more callers away from argv_array nameLibravatar Jeff King1-4/+4
We eventually want to drop the argv_array name and just use strvec consistently. There's no particular reason we have to do it all at once, or care about interactions between converted and unconverted bits. Because of our preprocessor compat layer, the names are interchangeable to the compiler (so even a definition and declaration using different names is OK). This patch converts remaining files from the first half of the alphabet, to keep the diff to a manageable size. The conversion was done purely mechanically with: git ls-files '*.c' '*.h' | xargs perl -i -pe ' s/ARGV_ARRAY/STRVEC/g; s/argv_array/strvec/g; ' and then selectively staging files with "git add '[abcdefghjkl]*'". We'll deal with any indentation/style fallouts separately. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-15gpg-interface: prefer check_signature() for GPG verificationLibravatar Hans Jerry Illikainen1-48/+49
This commit refactors the use of verify_signed_buffer() outside of gpg-interface.c to use check_signature() instead. It also turns verify_signed_buffer() into a file-local function since it's now only invoked internally by check_signature(). There were previously two globally scoped functions used in different parts of Git to perform GPG signature verification: verify_signed_buffer() and check_signature(). Now only check_signature() is used. The verify_signed_buffer() function doesn't guard against duplicate signatures as described by Michał Górny [1]. Instead it only ensures a non-erroneous exit code from GPG and the presence of at least one GOODSIG status field. This stands in contrast with check_signature() that returns an error if more than one signature is encountered. The lower degree of verification makes the use of verify_signed_buffer() problematic if callers don't parse and validate the various parts of the GPG status message themselves. And processing these messages seems like a task that should be reserved to gpg-interface.c with the function check_signature(). Furthermore, the use of verify_signed_buffer() makes it difficult to introduce new functionality that relies on the content of the GPG status lines. Now all operations that does signature verification share a single entry point to gpg-interface.c. This makes it easier to propagate changed or additional functionality in GPG signature verification to all parts of Git, without having odd edge-cases that don't perform the same degree of verification. [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-05Merge branch 'hi/gpg-use-check-signature'Libravatar Junio C Hamano1-49/+48
"git merge signed-tag" while lacking the public key started to say "No signature", which was utterly wrong. This regression has been reverted. * hi/gpg-use-check-signature: Revert "gpg-interface: prefer check_signature() for GPG verification"
2020-02-28Revert "gpg-interface: prefer check_signature() for GPG verification"Libravatar Junio C Hamano1-49/+48
This reverts commit 72b006f4bfd30b7c5037c163efaf279ab65bea9c, which breaks the end-user experience when merging a signed tag without having the public key. We should report "can't check because we have no public key", but the code with this change claimed that there was no signature.
2020-01-30Merge branch 'hi/gpg-mintrustlevel'Libravatar Junio C Hamano1-12/+79
gpg.minTrustLevel configuration variable has been introduced to tell various signature verification codepaths the required minimum trust level. * hi/gpg-mintrustlevel: gpg-interface: add minTrustLevel as a configuration option
2020-01-15gpg-interface: add minTrustLevel as a configuration optionLibravatar Hans Jerry Illikainen1-12/+79
Previously, signature verification for merge and pull operations checked if the key had a trust-level of either TRUST_NEVER or TRUST_UNDEFINED in verify_merge_signature(). If that was the case, the process die()d. The other code paths that did signature verification relied entirely on the return code from check_commit_signature(). And signatures made with a good key, irregardless of its trust level, was considered valid by check_commit_signature(). This difference in behavior might induce users to erroneously assume that the trust level of a key in their keyring is always considered by Git, even for operations where it is not (e.g. during a verify-commit or verify-tag). The way it worked was by gpg-interface.c storing the result from the key/signature status *and* the lowest-two trust levels in the `result` member of the signature_check structure (the last of these status lines that were encountered got written to `result`). These are documented in GPG under the subsection `General status codes` and `Key related`, respectively [1]. The GPG documentation says the following on the TRUST_ status codes [1]: """ These are several similar status codes: - TRUST_UNDEFINED <error_token> - TRUST_NEVER <error_token> - TRUST_MARGINAL [0 [<validation_model>]] - TRUST_FULLY [0 [<validation_model>]] - TRUST_ULTIMATE [0 [<validation_model>]] For good signatures one of these status lines are emitted to indicate the validity of the key used to create the signature. The error token values are currently only emitted by gpgsm. """ My interpretation is that the trust level is conceptionally different from the validity of the key and/or signature. That seems to also have been the assumption of the old code in check_signature() where a result of 'G' (as in GOODSIG) and 'U' (as in TRUST_NEVER or TRUST_UNDEFINED) were both considered a success. The two cases where a result of 'U' had special meaning were in verify_merge_signature() (where this caused git to die()) and in format_commit_one() (where it affected the output of the %G? format specifier). I think it makes sense to refactor the processing of TRUST_ status lines such that users can configure a minimum trust level that is enforced globally, rather than have individual parts of git (e.g. merge) do it themselves (except for a grace period with backward compatibility). I also think it makes sense to not store the trust level in the same struct member as the key/signature status. While the presence of a TRUST_ status code does imply that the signature is good (see the first paragraph in the included snippet above), as far as I can tell, the order of the status lines from GPG isn't well-defined; thus it would seem plausible that the trust level could be overwritten with the key/signature status if they were stored in the same member of the signature_check structure. This patch introduces a new configuration option: gpg.minTrustLevel. It consolidates trust-level verification to gpg-interface.c and adds a new `trust_level` member to the signature_check structure. Backward-compatibility is maintained by introducing a special case in verify_merge_signature() such that if no user-configurable gpg.minTrustLevel is set, then the old behavior of rejecting TRUST_UNDEFINED and TRUST_NEVER is enforced. If, on the other hand, gpg.minTrustLevel is set, then that value overrides the old behavior. Similarly, the %G? format specifier will continue show 'U' for signatures made with a key that has a trust level of TRUST_UNDEFINED or TRUST_NEVER, even though the 'U' character no longer exist in the `result` member of the signature_check structure. A new format specifier, %GT, is also introduced for users that want to show all possible trust levels for a signature. Another approach would have been to simply drop the trust-level requirement in verify_merge_signature(). This would also have made the behavior consistent with other parts of git that perform signature verification. However, requiring a minimum trust level for signing keys does seem to have a real-world use-case. For example, the build system used by the Qubes OS project currently parses the raw output from verify-tag in order to assert a minimum trust level for keys used to sign git tags [2]. [1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/doc/DETAILS;h=bd00006e933ac56719b1edd2478ecd79273eae72;hb=refs/heads/master [2] https://github.com/QubesOS/qubes-builder/blob/9674c1991deef45b1a1b1c71fddfab14ba50dccf/scripts/verify-git-tag#L43 Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-30gpg-interface: prefer check_signature() for GPG verificationLibravatar Hans Jerry Illikainen1-48/+49
This commit refactors the use of verify_signed_buffer() outside of gpg-interface.c to use check_signature() instead. It also turns verify_signed_buffer() into a file-local function since it's now only invoked internally by check_signature(). There were previously two globally scoped functions used in different parts of Git to perform GPG signature verification: verify_signed_buffer() and check_signature(). Now only check_signature() is used. The verify_signed_buffer() function doesn't guard against duplicate signatures as described by Michał Górny [1]. Instead it only ensures a non-erroneous exit code from GPG and the presence of at least one GOODSIG status field. This stands in contrast with check_signature() that returns an error if more than one signature is encountered. The lower degree of verification makes the use of verify_signed_buffer() problematic if callers don't parse and validate the various parts of the GPG status message themselves. And processing these messages seems like a task that should be reserved to gpg-interface.c with the function check_signature(). Furthermore, the use of verify_signed_buffer() makes it difficult to introduce new functionality that relies on the content of the GPG status lines. Now all operations that does signature verification share a single entry point to gpg-interface.c. This makes it easier to propagate changed or additional functionality in GPG signature verification to all parts of Git, without having odd edge-cases that don't perform the same degree of verification. [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-23gpg-interface: limit search for primary key fingerprintLibravatar Hans Jerry Illikainen1-6/+18
The VALIDSIG status line from GnuPG with --status-fd is documented to have 9 required and 1 optional fields [1]. The final, and optional, field is used to specify the fingerprint of the primary key that made the signature in case it was made by a subkey. However, this field is only available for OpenPGP signatures; not for CMS/X.509. If the VALIDSIG status line does not have the optional 10th field, the current code will continue reading onto the next status line. And this is the case for non-OpenPGP signatures [1]. The consequence is that a subsequent status line may be considered as the "primary key" for signatures that does not have an actual primary key. Limit the search of these 9 or 10 fields to the single line to avoid this problem. If the 10th field is missing, report that there is no primary key fingerprint. [Reference] [1] GnuPG Details, General status codes https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=6ce340e8c04794add995e84308bb3091450bd28f;hb=HEAD#l483 The documentation says: VALIDSIG <args> The args are: - <fingerprint_in_hex> - <sig_creation_date> - <sig-timestamp> - <expire-timestamp> - <sig-version> - <reserved> - <pubkey-algo> - <hash-algo> - <sig-class> - [ <primary-key-fpr> ] This status indicates that the signature is cryptographically valid. [...] PRIMARY-KEY-FPR is the fingerprint of the primary key or identical to the first argument. The primary-key-fpr parameter is used for OpenPGP and not available for CMS signatures. [...] Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-23gpg-interface: refactor the free-and-xmemdupz patternLibravatar Hans Jerry Illikainen1-7/+15
Introduce a static replace_cstring() function to simplify repeated pattern of free-and-xmemdupz() for GPG status line parsing. This also helps us avoid potential memleaks if parsing of new status lines are introduced in the future. Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-07-16gpg-interface: do not scan past the end of bufferLibravatar Steven Roberts1-0/+3
If the GPG output ends with trailing blank lines, after skipping them over inside the loop to find the terminating NUL at the end, the loop ends up looking for the next line, starting past the end. Signed-off-by: Steven Roberts <sroberts@fenderq.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-11-03Merge branch 'mg/gpg-fingerprint'Libravatar Junio C Hamano1-12/+45
New "--pretty=format:" placeholders %GF and %GP that show the GPG key fingerprints have been invented. * mg/gpg-fingerprint: gpg-interface.c: obtain primary key fingerprint as well gpg-interface.c: support getting key fingerprint via %GF format gpg-interface.c: use flags to determine key/signer info presence
2018-11-03Merge branch 'mg/gpg-parse-tighten'Libravatar Junio C Hamano1-29/+61
Detect and reject a signature block that has more than one GPG signature. * mg/gpg-parse-tighten: gpg-interface.c: detect and reject multiple signatures on commits
2018-10-23gpg-interface.c: obtain primary key fingerprint as wellLibravatar Michał Górny1-1/+15
Obtain the primary key fingerprint off VALIDSIG status message, and expose it via %GP format. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-23gpg-interface.c: support getting key fingerprint via %GF formatLibravatar Michał Górny1-1/+13
Support processing VALIDSIG status that provides additional information for valid signatures. Use this information to propagate signing key fingerprint and expose it via %GF pretty format. This format can be used to build safer key verification systems that verify the key via complete fingerprint rather than short/long identifier provided by %GK. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-23gpg-interface.c: use flags to determine key/signer info presenceLibravatar Michał Górny1-10/+17
Replace the logic used to determine whether key and signer information is present to use explicit flags in sigcheck_gpg_status[] array. This is more future-proof, since it makes it possible to add additional statuses without having to explicitly update the conditions. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-22gpg-interface.c: detect and reject multiple signatures on commitsLibravatar Michał Górny1-29/+61
GnuPG supports creating signatures consisting of multiple signature packets. If such a signature is verified, it outputs all the status messages for each signature separately. However, git currently does not account for such scenario and gets terribly confused over getting multiple *SIG statuses. For example, if a malicious party alters a signed commit and appends a new untrusted signature, git is going to ignore the original bad signature and report untrusted commit instead. However, %GK and %GS format strings may still expand to the data corresponding to the original signature, potentially tricking the scripts into trusting the malicious commit. Given that the use of multiple signatures is quite rare, git does not support creating them without jumping through a few hoops, and finally supporting them properly would require extensive API improvement, it seems reasonable to just reject them at the moment. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-20Merge branch 'jc/gpg-status'Libravatar Junio C Hamano1-1/+2
"git verify-tag" and "git verify-commit" have been taught to use the exit status of underlying "gpg --verify" to signal bad or untrusted signature they found. * jc/gpg-status: gpg-interface: propagate exit status from gpg back to the callers
2018-08-09gpg-interface: propagate exit status from gpg back to the callersLibravatar Junio C Hamano1-1/+2
When gpg-interface API unified support for signature verification codepaths for signed tags and signed commits in mid 2015 at around v2.6.0-rc0~114, we accidentally loosened the GPG signature verification. Before that change, signed commits were verified by looking for "G"ood signature from GPG, while ignoring the exit status of "gpg --verify" process, while signed tags were verified by simply passing the exit status of "gpg --verify" through. The unified code we currently have ignores the exit status of "gpg --verify" and returns successful verification when the signature matches an unexpired key regardless of the trust placed on the key (i.e. in addition to "G"ood ones, we accept "U"ntrusted ones). Make these commands signal failure with their exit status when underlying "gpg --verify" (or the custom command specified by "gpg.program" configuration variable) does so. This essentially changes their behaviour in a backward incompatible way to reject signatures that have been made with untrusted keys even if they correctly verify, as that is how "gpg --verify" behaves. Note that the code still overrides a zero exit status obtained from "gpg" (or gpg.program) if the output does not say the signature is good or computes correctly but made with untrusted keys, to catch a poorly written wrapper around "gpg" the user may give us. We could exclude "U"ntrusted support from this fallback code, but that would be making two backward incompatible changes in a single commit, so let's avoid that for now. A follow-up change could do so if desired. Helped-by: Vojtech Myslivec <vojtech.myslivec@nic.cz> Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-18gpg-interface: introduce new signature format "x509" using gpgsmLibravatar Henning Schild1-0/+15
This commit allows git to create and check x509 type signatures using gpgsm. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-18gpg-interface: introduce new config to select per gpg format programLibravatar Henning Schild1-1/+1
Supporting multiple signing formats we will have the need to configure a custom program each. Add a new config value to cater for that. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-18gpg-interface: do not hardcode the key string len anymoreLibravatar Henning Schild1-3/+4
gnupg does print the keyid followed by a space and the signer comes next. The same pattern is also used in gpgsm, but there the key length would be 40 instead of 16. Instead of hardcoding the expected length, find the first space and calculate it. Input that does not match the expected format will be ignored now, before we jumped to found+17 which might have been behind the end of an unexpected string. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-18gpg-interface: introduce an abstraction for multiple gpg formatsLibravatar Henning Schild1-21/+67
Create a struct that holds the format details for the supported formats. At the moment that is still just "openpgp". This commit prepares for the introduction of more formats, that might use other programs and match other signatures. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-17gpg-interface: add new config to select how to sign a commitLibravatar Henning Schild1-0/+10
Add "gpg.format" where the user can specify which type of signature to use for commits. At the moment only "openpgp" is supported and the value is not even used. This commit prepares for a new types of signatures. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-11gpg-interface: make parse_gpg_output static and remove from interface headerLibravatar Henning Schild1-1/+1
Turn parse_gpg_output into a static function, the only outside user was migrated in an earlier commit. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-16gpg-interface: find the last gpg signature lineLibravatar Jeff King1-3/+9
A signed tag has a detached signature like this: object ... [...more header...] This is the tag body. -----BEGIN PGP SIGNATURE----- [opaque gpg data] -----END PGP SIGNATURE----- Our parser finds the _first_ line that appears to start a PGP signature block, meaning we may be confused by a signature (or a signature-like line) in the actual body. Let's keep parsing and always find the final block, which should be the detached signature over all of the preceding content. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-16gpg-interface: extract gpg line matching helperLibravatar Jeff King1-2/+7
Let's separate the actual line-by-line parsing of signatures from the notion of "is this a gpg signature line". That will make it easier to do more refactoring of this loop in future patches. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-16gpg-interface: fix const-correctness of "eol" pointerLibravatar Jeff King1-2/+1
We accidentally shed the "const" of our buffer by passing it through memchr. Let's fix that, and while we're at it, move our variable declaration inside the loop, which is the only place that uses it. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-16gpg-interface: use size_t for signature buffer sizeLibravatar Jeff King1-1/+1
Even though our object sizes (from which these buffers would come) are typically "unsigned long", this is something we'd like to eventually fix (since it's only 32-bits even on 64-bit Windows). It makes more sense to use size_t when taking an in-memory buffer. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-16gpg-interface: modernize function declarationsLibravatar Jeff King1-17/+0
Let's drop "extern" from our declarations, which brings us in line with our modern style guidelines. While we're here, let's wrap some of the overly long lines, and move docstrings for public functions to their declarations, since they document the interface. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-16gpg-interface: handle bool user.signingkeyLibravatar Jeff King1-0/+6
The config handler for user.signingkey does not check for a boolean value, and thus: git -c user.signingkey tag will segfault. We could fix this and even shorten the code by using git_config_string(). But our set_signing_key() helper is used by other code outside of gpg-interface.c, so we must keep it (and we may as well use it, because unlike git_config_string() it does not leak when we overwrite an old value). Ironically, the handler for gpg.program just below _could_ use git_config_string() but doesn't. But since we're going to touch that in a future patch, we'll leave it alone for now. We will add some whitespace and returns in preparation for adding more config keys, though. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06tempfile: auto-allocate tempfiles on heapLibravatar Jeff King1-8/+8
The previous commit taught the tempfile code to give up ownership over tempfiles that have been renamed or deleted. That makes it possible to use a stack variable like this: struct tempfile t; create_tempfile(&t, ...); ... if (!err) rename_tempfile(&t, ...); else delete_tempfile(&t); But doing it this way has a high potential for creating memory errors. The tempfile we pass to create_tempfile() ends up on a global linked list, and it's not safe for it to go out of scope until we've called one of those two deactivation functions. Imagine that we add an early return from the function that forgets to call delete_tempfile(). With a static or heap tempfile variable, the worst case is that the tempfile hangs around until the program exits (and some functions like setup_shallow_temporary rely on this intentionally, creating a tempfile and then leaving it for later cleanup). But with a stack variable as above, this is a serious memory error: the variable goes out of scope and may be filled with garbage by the time the tempfile code looks at it. Let's see if we can make it harder to get this wrong. Since many callers need to allocate arbitrary numbers of tempfiles, we can't rely on static storage as a general solution. So we need to turn to the heap. We could just ask all callers to pass us a heap variable, but that puts the burden on them to call free() at the right time. Instead, let's have the tempfile code handle the heap allocation _and_ the deallocation (when the tempfile is deactivated and removed from the list). This changes the return value of all of the creation functions. For the cleanup functions (delete and rename), we'll add one extra bit of safety: instead of taking a tempfile pointer, we'll take a pointer-to-pointer and set it to NULL after freeing the object. This makes it safe to double-call functions like delete_tempfile(), as the second call treats the NULL input as a noop. Several callsites follow this pattern. The resulting patch does have a fair bit of noise, as each caller needs to be converted to handle: 1. Storing a pointer instead of the struct itself. 2. Passing the pointer instead of taking the struct address. 3. Handling a "struct tempfile *" return instead of a file descriptor. We could play games to make this less noisy. For example, by defining the tempfile like this: struct tempfile { struct heap_allocated_part_of_tempfile { int fd; ...etc } *actual_data; } Callers would continue to have a "struct tempfile", and it would be "active" only when the inner pointer was non-NULL. But that just makes things more awkward in the long run. There aren't that many callers, so we can simply bite the bullet and adjust all of them. And the compiler makes it easy for us to find them all. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06tempfile: do not delete tempfile on failed closeLibravatar Jeff King1-1/+1
When close_tempfile() fails, we delete the tempfile and reset the fields of the tempfile struct. This makes it easier for callers to return without cleaning up, but it also makes this common pattern: if (close_tempfile(tempfile)) return error_errno("error closing %s", tempfile->filename.buf); wrong, because the "filename" field has been reset after the failed close. And it's not easy to fix, as in many cases we don't have another copy of the filename (e.g., if it was created via one of the mks_tempfile functions, and we just have the original template string). Let's drop the feature that a failed close automatically deletes the file. This puts the burden on the caller to do the deletion themselves, but this isn't that big a deal. Callers which do: if (write(...) || close_tempfile(...)) { delete_tempfile(...); return -1; } already had to call delete when the write() failed, and so aren't affected. Likewise, any caller which just calls die() in the error path is OK; we'll delete the tempfile during the atexit handler. Because this patch changes the semantics of close_tempfile() without changing its signature, all callers need to be manually checked and converted to the new scheme. This patch covers all in-tree callers, but there may be others for not-yet-merged topics. To catch these, we rename the function to close_tempfile_gently(), which will attract compile-time attention to new callers. (Technically the original could be considered "gentle" already in that it didn't die() on errors, but this one is even more so). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06always check return value of close_tempfileLibravatar Jeff King1-2/+2
If close_tempfile() encounters an error, then it deletes the tempfile and resets the "struct tempfile". But many code paths ignore the return value and continue to use the tempfile. Instead, we should generally treat this the same as a write() error. Note that in the postimage of some of these cases our error message will be bogus after a failed close because we look at tempfile->filename (either directly or via get_tempfile_path). But after the failed close resets the tempfile object, this is guaranteed to be the empty string. That will be addressed in a future patch (because there are many more cases of the same problem than just these instances). Note also in the hunk in gpg-interface.c that it's fine to call delete_tempfile() in the error path, even if close_tempfile() failed and already deleted the file. The tempfile code is smart enough to know the second deletion is a noop. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06verify_signed_buffer: prefer close_tempfile() to close()Libravatar Jeff King1-1/+1
We do a manual close() on the descriptor provided to us by mks_tempfile. But this runs contrary to the advice in tempfile.h, which notes that you should always use close_tempfile(). Otherwise the descriptor may be reused without the tempfile object knowing it, and the later call to delete_tempfile() could close a random descriptor. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-24Merge branch 'ab/free-and-null'Libravatar Junio C Hamano1-10/+5
A common pattern to free a piece of memory and assign NULL to the pointer that used to point at it has been replaced with a new FREE_AND_NULL() macro. * ab/free-and-null: *.[ch] refactoring: make use of the FREE_AND_NULL() macro coccinelle: make use of the "expression" FREE_AND_NULL() rule coccinelle: add a rule to make "expression" code use FREE_AND_NULL() coccinelle: make use of the "type" FREE_AND_NULL() rule coccinelle: add a rule to make "type" code use FREE_AND_NULL() git-compat-util: add a FREE_AND_NULL() wrapper around free(ptr); ptr = NULL
2017-06-16*.[ch] refactoring: make use of the FREE_AND_NULL() macroLibravatar Ævar Arnfjörð Bjarmason1-10/+5
Replace occurrences of `free(ptr); ptr = NULL` which weren't caught by the coccinelle rule. These fall into two categories: - free/NULL assignments one after the other which coccinelle all put on one line, which is functionally equivalent code, but very ugly. - manually spotted occurrences where the NULL assignment isn't right after the free() call. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-15config: don't include config.h by defaultLibravatar Brandon Williams1-0/+1
Stop including config.h by default in cache.h. Instead only include config.h in those files which require use of the config system. Signed-off-by: Brandon Williams <bmwill@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-10-12gpg-interface: use more status lettersLibravatar Michael J Gruber1-3/+10
According to gpg2's doc/DETAILS: For each signature only one of the codes GOODSIG, BADSIG, EXPSIG, EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted. gpg1 ("classic") behaves the same (although doc/DETAILS differs). Currently, we parse gpg's status output for GOODSIG, BADSIG and trust information and translate that into status codes G, B, U, N for the %G? format specifier. git-verify-* returns success in the GOODSIG case only. This is somewhat in disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG, but we err on the very safe side. Introduce additional status codes E, X, Y, R for ERRSIG, EXPSIG, EXPKEYSIG, and REVKEYSIG so that a user of %G? gets more information about the absence of a 'G' on first glance. Requested-by: Alex <agrambot@gmail.com> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-08-16Merge branch 'lt/gpg-show-long-key-in-signature-verification-maint' into ↵Libravatar Junio C Hamano1-0/+1
lt/gpg-show-long-key-in-signature-verification Linus's original was rebased to apply to the maintenance track just in case binary distributors that are stuck in the past want to take it to their older codebase. Let's merge it up to more modern codebase that has Peff's gpg-interface clean-up topic that appeared after Git 2.9 was tagged. * lt/gpg-show-long-key-in-signature-verification-maint: gpg-interface: prefer "long" key format output when verifying pgp signatures
2016-08-16gpg-interface: prefer "long" key format output when verifying pgp signaturesLibravatar Linus Torvalds1-2/+2
Yes, gpg2 already uses the long format by default, but most distributions seem to still have "gpg" be the older 1.x version due to compatibility reasons. And older versions of gpg only show the 32-bit short ID, which is quite insecure. This doesn't actually matter for the _verification_ itself: if the verification passes, the pgp signature is good. But if you don't actually have the key yet, and want to fetch it, or you want to check exactly which key was used for verification and want to check it, we should specify the key with more precision. In fact, we should preferentially specify the whole key fingerprint, but gpg doesn't actually support that. Which is really quite sad. Showing the "long" format improves things to at least show 64 bits of the fingerprint. That's a lot better, even if it's not perfect. This change the log format for "git log --show-signature" from commit 2376d31787760af598db23bb3982a57419854e5c merged tag 'v2.9.3' gpg: Signature made Fri 12 Aug 2016 09:17:59 AM PDT using RSA key ID 96AFE6CB gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" gpg: aka "Junio C Hamano <jch@google.com>" gpg: aka "Junio C Hamano <junio@pobox.com>" Merge: 2807cd7b25af e0c1ceafc5be Author: Junio C Hamano <gitster@pobox.com> Date: Fri Aug 12 10:02:18 2016 -0700 to commit 2376d31787760af598db23bb3982a57419854e5c merged tag 'v2.9.3' gpg: Signature made Fri 12 Aug 2016 09:17:59 AM PDT gpg: using RSA key B0B5E88696AFE6CB gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" gpg: aka "Junio C Hamano <jch@google.com>" gpg: aka "Junio C Hamano <junio@pobox.com>" Merge: 2807cd7b25af e0c1ceafc5be Author: Junio C Hamano <gitster@pobox.com> Date: Fri Aug 12 10:02:18 2016 -0700 (note the longer key ID, but also the reflowing of the text) and also changes the format in the merge messages when merging a signed tag. If you already use gpg2 (either because it's installed by default, or because you have set your gpg_program configuration to point to gpg2), that already used the long format, you'll also see a change: it will now have the same formatting as gpg 1.x, and the verification string looks something like gpg: Signature made Sun 24 Jul 2016 12:24:02 PM PDT gpg: using RSA key 79BE3E4300411886 gpg: Good signature from "Linus Torvalds <torvalds@linux-foundation.org>" [ultimate] where it used to be on one line: gpg: Signature made Sun 24 Jul 2016 12:24:02 PM PDT using RSA key ID 79BE3E4300411886 gpg: Good signature from "Linus Torvalds <torvalds@linux-foundation.org>" [ultimate] so there is certainly a chance this could break some automated scripting. But the 32-bit key ID's really are broken. Also note that because of the differences between gpg-1.x and gpg-2.x, hopefully any scripted key ID parsing code (if such code exists) is already flexible enough to not care. This was triggered by the fact that the "evil32" project keys ended up leaking to the public key servers, so now there are 32-bit aliases for just about every open source developer that you can easily get by mistake if you use the 32-bit short ID format. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-17gpg-interface: check gpg signature creation statusLibravatar Michael J Gruber1-2/+6
When we create a signature, it may happen that gpg returns with "success" but not with an actual detached signature on stdout. Check for the correct signature creation status to catch these cases better. Really, --status-fd parsing is the only way to check gpg status reliably. We do the same for verify already. Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-17sign_buffer: use pipe_commandLibravatar Jeff King1-19/+5
Similar to the prior commit for verify_signed_buffer, the motivation here is both to make the code simpler, and to avoid any possible deadlocks with gpg. In this case we have the same "write to stdin, then read from stdout" that the verify case had. This is unlikely to be a problem in practice, since stdout has the detached signature, which it cannot compute until it has read all of stdin (if it were a non-detached signature, that would be a problem, though). We don't read from stderr at all currently. However, we will want to in a future patch, so this also prepares us there (and in that case gpg _does_ write before reading all of the input, though again, it is unlikely that a key uid will fill up a pipe buffer). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-17verify_signed_buffer: use pipe_commandLibravatar Jeff King1-19/+3
This is shorter and should make the function easier to follow. But more importantly, it removes the possibility of any deadlocks based on reading or writing to gpg. It's not clear if such a deadlock is possible in practice. We do write the whole payload before reading anything, so we could deadlock there. However, in practice gpg will need to read our whole input to verify the signature, so it will drain our payload first. It could write an error to stderr before reading, but it's unlikely that such an error wouldn't be followed by it immediately exiting, or that the error would actually be larger than a pipe buffer. On the writing side, we drain stderr (with the human-readable output) in its entirety before reading stdout (with the status-fd data). Running strace on "gpg --verify" does show interleaved output on the two descriptors: write(2, "gpg: ", 5) = 5 write(2, "Signature made Thu 16 Jun 2016 0"..., 73) = 73 write(1, "[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj"..., 66) = 66 write(1, "[GNUPG:] GOODSIG 69808639F9430ED"..., 60) = 60 write(2, "gpg: ", 5) = 5 write(2, "Good signature from \"Jeff King <"..., 47) = 47 write(2, "\n", 1) = 1 write(2, "gpg: ", 5) = 5 write(2, " aka \"Jeff King <"..., 49) = 49 write(2, "\n", 1) = 1 write(1, "[GNUPG:] VALIDSIG C49CE24156AF08"..., 135) = 135 write(1, "[GNUPG:] TRUST_ULTIMATE\n", 24) = 24 The second line written to stdout there contains the signer's UID, which can be arbitrarily long. If it fills the pipe buffer, then gpg would block writing to its stdout, while we are blocked trying to read its stderr. In practice, GPG seems to limit UIDs to 2048 bytes, so unless your pipe buffer size is quite small, or unless gpg does not enforce the limit under some conditions, this seems unlikely in practice. Still, it is not hard for us to be cautious and just use pipe_command. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-17verify_signed_buffer: use tempfile objectLibravatar Jeff King1-8/+13
We use git_mkstemp to create a temporary file, and try to clean it up in all exit paths from the function. But that misses any cases where we die by signal, or by calling die() in a sub-function. In addition, we missed one of the exit paths. Let's convert to using a tempfile object, which handles the hard cases for us, and add the missing cleanup call. Note that we would not simply want to rely on program exit to catch our missed cleanup, as this function may be called many times in a single program (for the same reason, we use a static tempfile instead of heap-allocating a new one; that gives an upper bound on our memory usage). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-17verify_signed_buffer: drop pbuf variableLibravatar Jeff King1-5/+4
If our caller gave us a non-NULL gpg_status parameter, we write the gpg status into their strbuf. If they didn't, then we write it to a temporary local strbuf (since we still need to look at it). The variable "pbuf" adds an extra layer of indirection so that the rest of the function can just access whichever is appropriate. However, the name "pbuf" isn't very descriptive, and it's easy to get confused about what is supposed to be in it (especially because we are reading both "status" and "output" from gpg). Rather than give it a more descriptive name, we can just use gpg_status as our indirection pointer. Either it points to the caller's input, or we can point it directly to our temporary buffer. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-17gpg-interface: use child_process.argsLibravatar Jeff King1-10/+9
Our argv allocations are relatively straightforward, but this avoids us having to manually keep the count up to date (or create new to-be-replaced slots in the declaration) when we add new arguments. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-05-17Merge branch 'nd/error-errno'Libravatar Junio C Hamano1-4/+2
The code for warning_errno/die_errno has been refactored and a new error_errno() reporting helper is introduced. * nd/error-errno: (41 commits) wrapper.c: use warning_errno() vcs-svn: use error_errno() upload-pack.c: use error_errno() unpack-trees.c: use error_errno() transport-helper.c: use error_errno() sha1_file.c: use {error,die,warning}_errno() server-info.c: use error_errno() sequencer.c: use error_errno() run-command.c: use error_errno() rerere.c: use error_errno() and warning_errno() reachable.c: use error_errno() mailmap.c: use error_errno() ident.c: use warning_errno() http.c: use error_errno() and warning_errno() grep.c: use error_errno() gpg-interface.c: use error_errno() fast-import.c: use error_errno() entry.c: use error_errno() editor.c: use error_errno() diff-no-index.c: use error_errno() ...
2016-05-09gpg-interface.c: use error_errno()Libravatar Nguyễn Thái Ngọc Duy1-4/+2
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-04-06builtin/verify-tag.c: ignore SIGPIPE in gpg-interfaceLibravatar Santiago Torres1-0/+2
The verify_signed_buffer() function may trigger a SIGPIPE when the GPG child process terminates early (due to a bad keyid, for example) and Git tries to write to it afterwards. Previously, ignoring SIGPIPE was done in builtin/verify-tag.c to avoid this issue. However, any other caller who wants to call verify_signed_buffer() would have to do the same. Use sigchain_push(SIGPIPE, SIG_IGN) in verify_signed_buffer(), pretty much like in sign_buffer(), so that any caller is not required to perform this task. This will avoid possible mistakes by further developers using verify_signed_buffer(). Signed-off-by: Santiago Torres <santiago@nyu.edu> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>