Open Bug 1147369 Opened 9 years ago Updated 4 months ago

Potentially modify libotr to use nss instead of gcrypt

Categories

(Chat Core :: Security: OTR, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: clokep, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

Attached patch WIP v1 (obsolete) — Splinter Review
Handle building libotr and dependent libraries in the build system.

Note that libotr depends on gcrypt, which depends in gpg-error. It is undesirable to have multiple crypto libraries included in Instantbird/Thunderbird and we'd prefer to just use nss. Unfortunately libotr does not allow for pluggable crypto libraries. The idea (at least that this patch starts on) is adding a shim which redirects gcrypt libraries to Mozilla/NSS versions.

A previous attempt at just building everything is at https://bitbucket.org/clokep/comm-central-patches/src/83d5268aa63c4138c9bc7b1b90ed5ef84ed87f4a/otr-libs?at=default

No libotr/gcrypt/gpg-error header/c files are modified in this patch, just moz.build/Makefile.in files + gcrypt-shim.cpp. There's also some stuff in here which will need to get removed before check-in: a couple of list of symbols we would need to define.
Attached patch libotr.patch (obsolete) — Splinter Review
Attaching the current progress. All the symbols should be implemented. There're likely many kinks to be worked out.

I've added gcrypt's sexp.c but also needed is libgpg-error.
Attached patch libotr.patch squashed (obsolete) — Splinter Review
The three commits squashed down into a single patch.
Comment on attachment 8637766 [details] [diff] [review]
libotr.patch squashed

Review of attachment 8637766 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/libraries/gcrypt/gcrypt-shim.cpp
@@ +38,5 @@
> +   gcry_free () to release memory allocated by libgcrypt. */
> +// TODO Should this use the libotr ones / secure this.
> +// TODO Add memory reporting.
> +void *gcry_malloc_secure(size_t n) {
> +  return moz_malloc(n);

No longer exists, just use malloc.

@@ +41,5 @@
> +void *gcry_malloc_secure(size_t n) {
> +  return moz_malloc(n);
> +}
> +void  gcry_free(void *a) {
> +  return moz_free(a);

Same here.
Attached patch More changes while compiling (obsolete) — Splinter Review
At this point it looks like compiling of this code was never attempted. Please take a look at the changes we've made and work on getting this to compile properly. We should be available for questions.
Comment on attachment 8637820 [details] [diff] [review]
More changes while compiling

Review of attachment 8637820 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/libraries/gcrypt/gcrypt-shim.c
@@ +248,5 @@
>  };
>  
>  // Always used with the same settings, AES in CTR Mode. Let's assert that
>  // and then just move forward. No need to generalize anything.
> +gcry_error_t gcry_cipher_open(gcry_cipher_hd_t hd, int algo, int mode,

That looks wrong.
Your build was successful!
Attached patch Combined patchSplinter Review
Folded all the previous attachment into one, and tweaked 3 lines so that gcrypt-shim.c could compile. This still doesn't link.
Attachment #8583060 - Attachment is obsolete: true
Attachment #8637695 - Attachment is obsolete: true
Attachment #8637766 - Attachment is obsolete: true
Attachment #8637815 - Attachment is obsolete: true
Attachment #8637820 - Attachment is obsolete: true
Attachment #8638174 - Attachment is obsolete: true
I'm trying to understand the status of this bug, which has last gotten activity mid 2015.

Apparently you had attempted to avoid the use of gcrypt, by using the "shim" code to map gcrypt calls to NSS calls.

However, I see that later in 2015, the Tor project released Tor Messenger, which bundled libgcrypt. I suspect this means that Tor Messenger didn't use the shim, and accepted the need to depend on both the NSS and gcrypt libraries. Is that correct?
(In reply to Kai Engert (:kaie:) from comment #10)
> Tor Messenger didn't use
> the shim, and accepted the need to depend on both the NSS and gcrypt
> libraries. Is that correct?

Correct.
I'm updating the bug summary to better describe the work that is attempted here.
Summary: Add libotr to build system → Potentially modify libotr to use nss instead of gcrypt
See Also: → 954310

I've experimented with the attached patch, to see what's missing for this approach.

(1)
We still have several undefined references to symbols from the gcrypt and gpg-error libraries,
so there's more work left to be done to make the shim complete.

(2)
The current shim makes a shortcut regarding the secure allocation functions. We must not simply map the allocation functions to standard malloc/free, but it must be changed to use the more secure versions from NSS that properly erases key material from memory.

(3)
The use of NSS low level crypto functions like AES_Encrypt needs to be modified.
NSS doesn't make those functions directly available from the shared libraries, instead, they must be accessed indirectly through function pointers, that are obtained using the freebl export function FREEBL_GetVector, which returns a vector with function pointers to all exported functions.

(4)
The mpi functions (arbitrary precision integers) aren't exported by NSS at all, they're only available from inside the NSS freebl library.
We'd have to identify a way how to use them. Because the code is contained in the Mozilla source tree, we could potentially compile them locally as part of the shim, and link them statically into the otr library.
Or we could ask the NSS team to export them in some way.

It's possible there's a more complete patch here,
https://github.com/arlolra/ctypes-otr/commits/master/patches/gcrypt-shim.cpp

but I don't remember where things stood. Just FYI

After many more changes, I'm able to link and run, and the JS code loads the OTR lib.
However, we're running into an assertion immediately.

Apparently the shim code assumes that the gcrypt-mpi and the nss-freebl-mpi code are compatible.
The attached patch uses this shim code:

gcry_error_t gcry_mpi_scan(gcry_mpi_t *ret_mpi, enum gcry_mpi_format format,
const void *buffer, size_t buflen,
size_t *nscanned) {
mp_err err = 0;
err = mp_init((mp_int *)*ret_mpi);
...

It uses a typecast from the gcrypt type "gcry_mpi_t" to the NSS-freebl type "mp_int",
but it seems the types are different:

struct gcry_mpi
{
int alloced; /* Array size (# of allocated limbs). /
int nlimbs; /
Number of valid limbs. /
int sign; /
Indicates a negative number and is also used
for opaque MPIs to store the length. /
unsigned int flags; /
Bit 0: Array to be allocated in secure memory space./
/
Bit 2: The limb is a pointer to some m_alloced data./
/
Bit 4: Immutable MPI - the MPI may not be modified. /
/
Bit 5: Constant MPI - the MPI will not be freed. */
mpi_limb_t d; / Array with the limbs */
};

vs.

typedef struct {
mp_sign sign; /* sign of this quantity /
mp_size alloc; /
how many digits allocated /
mp_size used; /
how many digits used */
mp_digit dp; / the digits themselves */
} mp_int;

The assertion was triggered by a NULL pointer given to mp_init, which it doesn't allow, so besides the type differences, the function call semantics are different, too.

Some NSS changes that were necessary in the Mozilla tree to make the above comm changes build.

This comment repeats comment 15, using markdown. (IMHO, the only bugzilla formatting, which simply kept whitespace indenting with a monospace font, was much simpler to use. From now on I'll have to worry about formatting every time I write a commnent, instead of simply focusing on the contents?)

After many more changes, I'm able to link and run, and the JS code loads the OTR lib.
However, we're running into an assertion immediately.

Apparently the shim code assumes that the gcrypt-mpi and the nss-freebl-mpi code are compatible.
The attached patch uses this shim code:

gcry_error_t gcry_mpi_scan(gcry_mpi_t *ret_mpi, enum gcry_mpi_format format,
                           const void *buffer, size_t buflen,
                           size_t *nscanned) {
  mp_err err = 0;
  err = mp_init((mp_int *)*ret_mpi);
  ...

It uses a typecast from the gcrypt type gcry_mpi_t to the NSS-freebl type mp_int,
but it seems the types are different:

struct gcry_mpi
{
  int alloced;         /* Array size (# of allocated limbs). */
  int nlimbs;          /* Number of valid limbs. */
  int sign;	       /* Indicates a negative number and is also used
		          for opaque MPIs to store the length.  */
  unsigned int flags; /* Bit 0: Array to be allocated in secure memory space.*/
                      /* Bit 2: The limb is a pointer to some m_alloced data.*/
                      /* Bit 4: Immutable MPI - the MPI may not be modified.  */
                      /* Bit 5: Constant MPI - the MPI will not be freed.  */
  mpi_limb_t *d;      /* Array with the limbs */
};

vs.

typedef struct {
    mp_sign sign;  /* sign of this quantity      */
    mp_size alloc; /* how many digits allocated  */
    mp_size used;  /* how many digits used       */
    mp_digit *dp;  /* the digits themselves      */
} mp_int;

The assertion was triggered by a NULL pointer given to mp_init, which it doesn't allow, so besides the type differences, the function call semantics are different, too.

No longer blocks: 954310
Type: defect → enhancement
Component: General → Security: OTR
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: