[cc:ed Eric Sunshine and the Git list since folks might be interested.. not quite [PATCH] ready yet, but it's not too far...] On Thu, Jul 25, 2013 at 6:00 PM, Jeremy Huddleston Sequoia <jeremyhu@xxxxxxxxx> wrote: > > On Jul 25, 2013, at 17:40, David Aguilar <davvid@xxxxxxxxx> wrote: > >> On Thu, Jul 25, 2013 at 4:19 PM, Jeremy Huddleston Sequoia >> <jeremyhu@xxxxxxxxx> wrote: >>> Thanks for finally backtracking on this decision(*) and deciding to take my CommonCrypto changes into git. You really should've attributed us for providing you with this patch, but at least they're there... >>> >>> --Jeremy >>> >>> *: http://git.661346.n2.nabble.com/PATCH-darwin-Use-CommonCrypto-to-compute-SHA1-td4706146.html#a4706906 >> >> Ah, sorry for not having seen that thread; I would have certainly >> liked to have credited you. >> >> There are still a few deprecation warnings when building on >> imap-send.c on 10.8 after these changes. We don't have a solution for >> those yet. The warnings are because we use things from >> openssl/x509v3.h. >> >> If you are aware of an upgrade path for that functionality then I'd be >> happy to work with the Git list to get them in (and properly attribute >> you, of course). > > > Hi David, > > Based on comparing deprecation warning from my changes and 1.8.3.4, it looks like you got about as far as I did (HMAC/CRAM-MD5 and SHA1). > > Looking back on my notes, I was intending to rewrite the connection code in imap-send.c using SecureTransport (<Security/SecureTransport.h>), but I hadn't actually started working on that. > > I also took a stab at replacing git's use of OpenSSL for base64 encode/decode, but I ran into some issues, and the bug has been sitting with the maintainers of CFData for a while. I've attached patch (which has probably bitrotted) and test-case for the bug. If you wouldn't mind, could you review the changes as I'm not confident that it's not just a bug in my code and would prefer some other eyes looking it over. > ~ $ clang test.c -lcrypto -framework Security -framework CoreFoundation > test.c:50:9: warning: 'EVP_DecodeBlock' is deprecated [-Wdeprecated-declarations] > o = EVP_DecodeBlock(out, (unsigned char *)in, strlen(in)); > ^ > 1 warning generated. > > ~ $ ./a.out > 49 <0606932735571463.1337625513@xxxxxxxxxxxxxxxxxxx> > 51 <0606932735571463.1337625513@xxxxxxxxxxxxxxxxxxx> > > Thanks, > Jeremy Hmm. strlen() on both outputs is the same, so it's correct in that respect, but it does seem like a bug in CFDataGetLength(). I don't think we deal with nulls inside the strings so that could paper over it, but it does seem kinda wrong. The reason is that base64 encoding requires padding of the data so that its length is a multiple of 3, and 51 is the next divisible by 3 number. The internals of the decoder is leaking into the interface it seems... I'm not sure what that would mean for backwards compatibility if CFDataGetLength() were changed. For Git, it seems that strlen(output) would work for imap-send's purposes. My assumption is that folks don't have NULL in their usernames and passwords, which I think is sane. I've inlined your original patch and test program below. Jeremy, can you help with a follow-up patch? The style should also be adjusted to match Git's; under_scores instead of camelCase, and tabs for indents. I can help roll a patch together next week if you don't beat me to it ;-) cheers, and thanks for the patch. -- David --- test.c --- #include <stdio.h> #include <Security/Security.h> #include <openssl/evp.h> #define die(...) abort() static int _EVP_DecodeBlock(unsigned char *out, const unsigned char *in, int in_len) { CFErrorRef error; SecTransformRef decoder; CFDataRef inData, outData; CFIndex length, result_len; decoder = SecDecodeTransformCreate(kSecBase64Encoding, &error); if (!decoder) { die("SecEncodeTransformCreate failed: %ld", (long)CFErrorGetCode(error)); } inData = CFDataCreate(kCFAllocatorDefault, in, in_len); SecTransformSetAttribute(decoder, kSecTransformInputAttributeName, inData, &error); if (error) { die("SecTransformSetAttribute failed: %ld", (long)CFErrorGetCode(error)); } outData = SecTransformExecute(decoder, &error); if (error) { die("SecTransformExecute failed: %ld", (long)CFErrorGetCode(error)); } length = CFDataGetLength(outData); CFDataGetBytes(outData, CFRangeMake(0, length), out); CFRelease(outData); CFRelease(inData); CFRelease(decoder); return (int)length; } int main() { int i; const char *in = "PDA2MDY5MzI3MzU1NzE0NjMuMTMzNzYyNTUxM0BjaWQub3V0ZXJzcXVhcmUub3JnPg=="; unsigned char out[1024]; int o; o = _EVP_DecodeBlock(out, (unsigned char *)in, strlen(in)); printf("%d %ld %s\n", o, strlen((const char *)out), out); o = EVP_DecodeBlock(out, (unsigned char *)in, strlen(in)); printf("%d %ld %s\n", o, strlen((const char *)out), out); return 0; } --- >8 --- original patch --- >8 --- commit c5d5a2cb24bc3248aa49076577131e550e2af529 Author: Jeremy Huddleston <jeremyhu@xxxxxxxxx> Date: Thu May 17 15:19:59 2012 -0700 <rdar://problem/11477310> Stop using OpenSSL for base64 encoding Signed-off-by: Jeremy Huddleston <jeremyhu@xxxxxxxxx> diff --git a/src/git/Makefile b/src/git/Makefile index 845e7c0..f3bb32f 100644 --- a/src/git/Makefile +++ b/src/git/Makefile @@ -1483,6 +1483,10 @@ else LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto endif endif +ifdef COMMONCRYPTO + # We use Security.framework for base64 encoding when CommonCrypto is enabled on Lion+ + LIB_4_CRYPTO += -framework Security -framework CoreFoundation +endif ifdef NEEDS_LIBICONV ifdef ICONVDIR BASIC_CFLAGS += -I$(ICONVDIR)/include diff --git a/src/git/imap-send.c b/src/git/imap-send.c index 1f4fde5..3901644 100644 --- a/src/git/imap-send.c +++ b/src/git/imap-send.c @@ -22,6 +22,15 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ +/* This block needs to come first, so git-compat-util.h can overwrite ctype.h */ +#ifdef USE_COMMONCRYPTO +#include <CommonCrypto/CommonCrypto.h> + +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070 +#include <Security/Security.h> +#endif +#endif + #include "cache.h" #include "exec_cmd.h" #include "run-command.h" @@ -33,10 +42,6 @@ typedef void *SSL; #include <openssl/hmac.h> #endif -#ifdef USE_COMMONCRYPTO -#include <CommonCrypto/CommonCrypto.h> -#endif - struct store_conf { char *name; const char *path; /* should this be here? its interpretation is driver-specific */ @@ -952,7 +957,7 @@ static void imap_close_store(struct store *ctx) free(ctx); } -#ifndef NO_OPENSSL +#if !defined(NO_OPENSSL) || (defined(USE_COMMONCRYPTO) && defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070) /* * hexchar() and cram() functions are based on the code from the isync @@ -963,6 +968,77 @@ static char hexchar(unsigned int b) return b < 10 ? '0' + b : 'a' + (b - 10); } +#if defined(USE_COMMONCRYPTO) && defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070 +#define EVP_DecodeBlock _EVP_DecodeBlock +#define EVP_EncodeBlock _EVP_EncodeBlock + +static int EVP_EncodeBlock(unsigned char *out, const unsigned char *in, int in_len) { + CFErrorRef error; + SecTransformRef encoder; + CFDataRef inData, outData; + CFIndex length; + + encoder = SecEncodeTransformCreate(kSecBase64Encoding, &error); + if (!encoder) { + die("SecEncodeTransformCreate failed: %ld", (long)CFErrorGetCode(error)); + } + + inData = CFDataCreate(kCFAllocatorDefault, in, in_len); + + SecTransformSetAttribute(encoder, kSecTransformInputAttributeName, inData, &error); + if (error) { + die("SecTransformSetAttribute failed: %ld", (long)CFErrorGetCode(error)); + } + + outData = SecTransformExecute(encoder, &error); + if (error) { + die("SecTransformExecute failed: %ld", (long)CFErrorGetCode(error)); + } + + length = CFDataGetLength(outData); + CFDataGetBytes(outData, CFRangeMake(0, length), out); + + CFRelease(outData); + CFRelease(inData); + CFRelease(encoder); + + return (int)length; +} + +static int EVP_DecodeBlock(unsigned char *out, const unsigned char *in, int in_len) { + CFErrorRef error; + SecTransformRef decoder; + CFDataRef inData, outData; + CFIndex length; + + decoder = SecDecodeTransformCreate(kSecBase64Encoding, &error); + if (!decoder) { + die("SecEncodeTransformCreate failed: %ld", (long)CFErrorGetCode(error)); + } + + inData = CFDataCreate(kCFAllocatorDefault, in, in_len); + + SecTransformSetAttribute(decoder, kSecTransformInputAttributeName, inData, &error); + if (error) { + die("SecTransformSetAttribute failed: %ld", (long)CFErrorGetCode(error)); + } + + outData = SecTransformExecute(decoder, &error); + if (error) { + die("SecTransformExecute failed: %ld", (long)CFErrorGetCode(error)); + } + + length = CFDataGetLength(outData); + CFDataGetBytes(outData, CFRangeMake(0, length), out); + + CFRelease(outData); + CFRelease(inData); + CFRelease(decoder); + + return (int)length; +} +#endif + #define ENCODED_SIZE(n) (4*((n+2)/3)) static char *cram(const char *challenge_64, const char *user, const char *pass) { --- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html