Allow passing in trustAnchors to newTLSClientAsyncStream #355

iffy · 2023-01-30T23:18:54Z

I'd like to be able to use my own certs, including self-signed certs in some TLS connections. This change lets me provide my own trust anchors.

Menduist · 2023-01-31T08:36:31Z

Be careful, you pushed tests/testasyncstream binary file

Menduist · 2023-01-31T09:49:23Z

chronos/streams/tlsstream.nim

@@ -465,8 +470,8 @@ proc newTLSClientAsyncStream*(rsource: AsyncStreamReader,
    sslEngineSetX509(res.ccontext.eng, addr res.xwc.vtable)
  else:
    sslClientInitFull(res.ccontext, addr res.x509,
-                      unsafeAddr MozillaTrustAnchors[0],
-                      uint(len(MozillaTrustAnchors)))
+                      unsafeAddr trustAnchors[0],


I'm not sure about the memory safety of this, seems like the caller could discard the anchors after calling this provider, and it will then lead to garbage memory

Totally agree.
First of all you should add assertion check at the beginning of this procedure.

doAssert(len(trustAnchors) > 0, "Some comment")

Would changing the parameter to trustAnchors: sink openArray[X509TrustAnchor] = MozillaTrustAnchors be sufficient? I'm not terribly familiar with how move/sink works, but my gut says this situation is what a sink is for.

This simple test program below seems to confirm that, but:

I don't know if it just gets lucky that the array's memory happens to still be intact

Or if it copies the whole contents of the array, which might be too expensive for newTLSClientAsyncStream to have to do each time.

type Thing = object name: string var globalThings: ptr Thing var globalThingsLen: int proc save(arr: sink openArray[Thing]) = globalThings = unsafeAddr arr[0] globalThingsLen = arr.len proc dosomething() = let arr = cast[ptr UncheckedArray[Thing]](globalThings) for i in 0..<globalThingsLen: echo arr[i] proc main = var myArrayOfThings: array[3, Thing] myArrayOfThings[0] = Thing(name: "alice") myArrayOfThings[1] = Thing(name: "bobby") myArrayOfThings[2] = Thing(name: "cathy") save myArrayOfThings myArrayOfThings[1] = Thing(name: "A walrus") dosomething() when isMainModule: main()

For me, that produces the following when compiled with --gc:orc, which is what I'd hope it would produce:

(name: "alice") (name: "bobby") (name: "cathy")

orc will only free stuff when they get out of scope, so as soon as main is finished, myArrayOfThings will no longer exist.
You can check by calling dosomething() after calling main() (which crashes on my laptop)

trustAnchors should be put on the heap. You might add a field of the seq[X509TrustAnchor] type to TLSAsyncStream so that it gets the same lifetime as ctx.

That's what I was thinking, but it seems like it could be an awful waste of memory to duplicate it for every stream compared with the current implementation that shares the anchors between streams. In my particular use case, I don't mind making a copy per stream (as I'll only have one trust anchor and very few client streams) but don't want to ruin the more common case in the process.

I suppose I could make it use a seq[X509TrustAnchor] if provided, otherwise use the const Mozilla anchors as-is?

List of trusted anchors is static array which should not be changed while process is running, seq[X509TrustAnchor] unable to satisfy this requirement, its mutable data structure. And i understand that we should somehow preserve this array in memory because of GC but storing it with every instance of TlsAsyncStream is also not a good solution. So i prefer to have openArray[X509TrustAnchor] and some comments in function description that you should be sure that specified trusted anchors list will be preserved in memory while TlsAsyncStream is alive.

iffy · 2023-01-31T13:24:25Z

Be careful, you pushed tests/testasyncstream binary file

Oops... in all my nim projects I gitignore these. I'll back it out.

iffy · 2023-02-01T15:21:19Z

Alright, I've pushed a new version that allows for passing an optional seq instead of an array.

I contemplated not using Option and instead using the Mozilla TAs if len(seq) == 0 but thought that could lead to users intending to use their own TAs and accidentally using Mozillas. But I understand if you don't want to introduce Options here.

Any other thoughts? Thank you for reviewing the mess I've put up so far :)

cheatfate · 2023-02-02T15:09:23Z

Option[T] will not going to be accepted.
Multiple waitFor (use await) in test is not going to be accepted.
openArray[T] should be used instead of seq[T] version, for many reasons. Trusted anchors database should be non-mutable data storage. seq[T] is mutable. So with openArray[T] we assume that certificates will be stored in process data section or present for whole process time. Nim will not allow us to create this specific limitations, but i prefer openArray here. Maybe there should be some comments left about how list of trusted anchors should be stored.
There should be assertion check for an empty array.

iffy · 2023-02-02T19:02:38Z

Option[T] will not going to be accepted.

👍

2. Multiple `waitFor` (use `await`) in test is not going to be accepted.

👍

3. `openArray[T]` should be used instead of `seq[T]` version, for many reasons. Trusted anchors database should be non-mutable data storage. `seq[T]` is mutable. So with `openArray[T]` we assume that certificates will be stored in process data section or present for whole process time. Nim will not allow us to create this specific limitations, but i prefer `openArray` here. Maybe there should be some comments left about how list of trusted anchors should be stored.

Both seq[T] and openArray[T] are mutable, though the array length is not. A caller could change either after passing them in.
Aren't seq passed by copy? So in this code each TlsAsyncStream gets a copy of the TAs, right? Or is it only a copy of the reference to the TA?
If a comment is the only thing standing between memory safety and not, I'm not sure how an array is better than a seq. I imagine there is a way to make it surely memory safe by forcing callers to construct and pass in a struct/object that owns a copy of the TA data (but doesn't make a copy per TlsAsyncStream). I may attempt this method, because I'd like memory safety to be more foolproof.

4. There should be assertion check for an empty array.

👍

cheatfate · 2023-02-02T21:30:31Z

TlsAsyncStream did not get copy of the TAs, bearssl using reference, so if seq will be garbage collected you will get SIGSEGV on certificate verification.
My idea is to have just single copy of TA for whole process (there is no reason to have multiple arrays with TAs around). And this copy must not be mutable (because of security reasons).
So maybe static could help us, but this will prohibit future work on cacert.pem parsing...

iffy · 2023-02-05T04:25:17Z

there is no reason to have multiple arrays with TAs around

@cheatfate My purpose in submitting this PR is to allow users of my program to set their TA at runtime, so static wouldn't work, right? I'm aware that the code currently doesn't copy TAs (it just uses the const Mozilla ones built in to nim-bearssl).

What about something like this?

type
  TrustAnchorStore = ref object
    tas: seq[X509TrustAnchor]

proc newTrustAnchorStore(anchors: openArray[X509TrustAnchor]): TrustAnchorStore
  new(result)
  for anchor in anchors:
    result.tas.add(copy(anchor)) # I'm not sure what the actual `copy` proc is, but this makes a copy

type
  TLSAsyncStream = ...
    ...
    trustAnchors*: TrustAnchorStore

proc newTLSClientAsyncStream*(
  ...
  trustAnchors = TrustAnchorStore | static[openArray[X509TrustAnchor]]
  ): TLSAsyncStream =
  ...

Callers of newTLSClientAsyncStream could still pass in a const array of TAs or pass nothing at all and get the Mozilla TAs. Or if they want runtime TAs, they could construct a TrustAnchorStore which will contain a copy of their TAs. They could reuse the same TrustAnchorStore for different calls, and there would be no danger of garbage collection because the TLSAsyncStream would keep a reference to the TrustAnchorStore.

Users might accidentally make too many TrustAnchorStores if they don't read the comments about reusing the store, but they can't accidentally get a segfault.

cheatfate · 2023-02-05T04:47:12Z

I dont want to allow people to change TAs at runtime at any cost but i wish to allow people to specify custom TAs.

We trying to keep version bundled in nim-bearssl up to date.
If somebody needs self-signed certificates, its possible to be done by using TLSFlags.
We going to work on cacert.pem parser and facility which will try to find this file in different OS.

What the real purpose of your PR, are you trying to alter official TAs list to add some "custom/malicious" authorities to be able to use this authorities to create new certificates?

iffy · 2023-02-05T05:25:04Z

What the real purpose of your PR, are you trying to alter official TAs list to add some "custom/malicious" authorities to be able to use this authorities to create new certificates?

Nothing malicious. I'm not trying to alter the official Mozilla TA list -- nothing in my PR does that. If my change is accepted, people using chronos will continue to use the Mozilla TAs unless they choose to supply a different TA list.

I'm making an application that can start websockets servers and clients (via nim-websock). Some of the users will want to use their own self-signed certificates to secure the connection between their two devices. The application will generate a self-signed cert for the server at runtime. They will then provide their client with the server's public key. The client will use that public key as the only TA when connecting to their server. That's it.

This is a use case described as "Non-CA trust anchors" in the 4th bullet point of these BearSSL docs: https://www.bearssl.org/x509.html#the-minimal-engine

Non-CA trust anchors implement direct trust: if the EE certificate matches a non-CA trust anchor (subjectDN identical to the anchor name, and same public key), then validation succeeds. The EE certificate itself is still fully decoded to allow for name extraction and matching, and key usage gathering.

The test case here shows pretty much exactly what I'm hoping to do, which is to make a TLS client that trusts only one TLS server: https://github.com/status-im/nim-chronos/pull/355/files#diff-7c38a4d2f5084806ec39ffde17cc1f0bccb0fd4802abecc5f9c2f8511208a71cR983

iffy · 2023-02-05T05:29:25Z

2. If somebody needs self-signed certificates, its possible to be done by using TLSFlags.

If there's a way to do this with TLSFlags that would be great! But I'm not seeing a way to specify a specific public key to trust here: https://github.com/status-im/nim-chronos/blob/master/chronos/streams/tlsstream.nim#L26 I only see options for not verifying certs.

cheatfate · 2023-02-06T18:11:23Z

Exactly, so you creating self-signed certificate, start your server with this certificate and use client with TLSFlags.NoVerifyHost to connect to your server with self-signed certificate. In such case client will not verify certificate using TAs.

iffy · 2023-02-06T19:20:13Z

I don't want to disable verification. I want the clients to verify that the server they are connecting to is the one they expect and the only server they trust.

iffy · 2023-02-06T19:26:38Z

I've pushed a new version that hopefully addresses the items from before:

Preserves existing behavior regarding using the Mozilla TAs -- if people don't pass in their own trustAnchors the MozillaTrustAnchors are used.
No use of Option
Using await instead of waitFor in the test
Asserts the size of TAs is > 0
Keeps a reference to custom TAs to prevent them from being garbage collected, but also allows reusing a TrustAnchorStore among several clients.

cheatfate · 2023-02-06T20:38:44Z

Looks like we unable to achieve consensus here, you proposing to include list of TAs with every TLSAsyncStream instance, so every object will occupy +200KB of memory without any reason, sorry but i can't accept it.

I don't want to disable verification. I want the clients to verify that the server they are connecting to is the one they expect and the only server they trust.

Clients are unable to verify that server they are connecting is the one they expect, it can happens only when server's certificate can be verified with some well-known list of TAs (like Mozilla's one). If you want to establish such type of trust you should buy official SSL certificate or get free one from https://letsencrypt.org/. And in such way you dont need to alter Mozilla's list of TAs.

iffy · 2023-02-06T21:15:19Z

so every object will occupy +200KB of memory without any reason

I don't believe this is true, but please correct my understanding if I'm wrong:

If you call newTLSClientAsyncStream without specifying trustAnchors you will use the MozillaTrustAnchors, not a copy of them, just like the current code does.

If you call newTLSClientAsyncStream with an instance of TrustAnchorStore, because TrustAnchorStore is a ref object and allocated on the heap, it will only store a reference, not a copy. Users who want to use a custom TrustAnchorStore should be advised that they should only make one store and pass the same instance to all newTLSClientAsyncStream calls. (I forgot to add a comment but I can add this).

var mystore = newTrustAnchorStore(...)
var client1 = newTLSClientAsyncStream(..., trustAnchors=mystore)
var client2 = newTLSClientAsyncStream(..., trustAnchors=mystore)

In the above code, client1 and client2 will share the same mystore, not separate copies, correct?

Clients are unable to verify that server they are connecting is the one they expect, it can happens only when server's certificate can be verified with some well-known list of TAs (like Mozilla's one).

This is not true. Neither TLS nor BearSSL require users to use a "well-known list of TAs." The set of TAs a server/client use is up to the server/client. It's convenient that chronos includes Mozilla's TAs for making connections, but it's not a requirement to only use Mozilla's TAs.

In the use-case I'm trying to support, my users aren't hosting things on the public Internet. They're not programmers and they wouldn't have a clue how to use letsencrypt. This is a desktop program that they run on a few computers. I want them to be able to connect their computers together without eavesdropping on their local/business network. It will work like this:

On computer 1, they press the "Start server" button.
This creates a self-signed TLS certificate.
On computer 2, they press the "Start client" button.
It asks them to put in the certificate of the server they want to connect to. They copy and paste it from one computer to the other.
Computer 2 can then connect securely to computer 1, with full TLS, including certificate validation.

In this scenario, the certs must be generated and anchors must be chosen at runtime. If I were to compile in the trusted anchors and then distribute it to my users, it would not be secure for them because

I would have the private keys they were using to secure their connection.
Anyone who gets the program could impersonate the server since all copies of the program contain the same keys.

And in such way you dont need to alter Mozilla's list of TAs.

I'm not trying to alter Mozilla's list of TAs. None of the code I've pushed alters Mozilla's list of TAs. I don't know why you keep claiming that.

What I'm proposing isn't strange. Many projects allow you to provide custom TAs. For example:

Python's requests has the verify="/path/to/cert.pem" option
As already mentioned, BearSSL lets you provide a "Non-CA trust anchor"
Rust tls lets you specify anchors with OwnedTrustAnchor
Go tls lets you use a custom root cert

tests/testasyncstream.nim

… reuse

tests/testasyncstream.nim

cheatfate · 2023-02-06T23:21:44Z

chronos/streams/tlsstream.nim

@@ -132,6 +136,12 @@ proc newTLSStreamProtocolError[T](message: T): ref TLSStreamProtocolError =
 proc raiseTLSStreamProtocolError[T](message: T) {.noreturn, noinline.} =
  raise newTLSStreamProtocolImpl(message)

+proc newTrustAnchorStore*(anchors: openArray[X509TrustAnchor]): TrustAnchorStore =
+  new(result)


Please avoid result usage and use this procedure.

proc new*(T: typedesc[TrustAnchorStore], anchors: openArray[X509TrustAnchor]): TrustAnchorStore = var res: seq[X509TrustAnchor] for anchor in anchors: res.add(anchor) doAssert(unsafeAddr(anchor) != unsafeAddr(res.anchors[^1]), "Anchors should be copied") TrustAnchorStore(anchors: res)

I tried the above, but running the test fails with:

Traceback (most recent call last) /Users/matt/.nimble/pkgs/unittest2-0.0.5/unittest2.nim(848) testasyncstream /Users/matt/lib/nim-chronos/tests/testasyncstream.nim(1022) runTest`gensym1864 /Users/matt/lib/nim-chronos/tests/testasyncstream.nim(987) checkTrustAnchors /Users/matt/lib/nim-chronos/chronos/streams/tlsstream.nim(143) newTrustAnchorStore SIGSEGV: Illegal storage access. (Attempt to read from nil?)

This also produce a SIGSEGV

proc newTrustAnchorStore*(anchors: openArray[X509TrustAnchor]): TrustAnchorStore = var res: TrustAnchorStore new(res) for anchor in anchors: res.anchors.add(anchor) doAssert(unsafeAddr(anchor) != unsafeAddr(result.anchors[^1]), "Anchors should be copied") return res

But the original (with new(result)) succeeds.

I'm not understanding something about how Nim allocates memory, I think. Does anyone else know what's wrong?

cheatfate · 2023-02-17T11:13:55Z

Sorry for long reply, but this small reproducible source working for me in 1.6

type
  X509TrustAnchor* = distinct string

  TrustAnchorStore* = ref object
    anchors: seq[X509TrustAnchor]

proc new*(T: typedesc[TrustAnchorStore],
          anchors: openArray[X509TrustAnchor]): TrustAnchorStore =
  var res: seq[X509TrustAnchor]
  for anchor in anchors:
    res.add(anchor)
    doAssert(unsafeAddr(anchor) != unsafeAddr(res[^1]),
             "Anchors should be copied")
  TrustAnchorStore(anchors: res)

when isMainModule:
  let store = TrustAnchorStore.new([X509TrustAnchor("test1"),
                                    X509TrustAnchor("test2"),
                                    X509TrustAnchor("test3"),
                                    X509TrustAnchor("test4")])
  echo string(store.anchors[0])

cheatfate · 2023-02-21T10:13:20Z

@iffy could you please update PR?

iffy · 2023-02-21T14:05:01Z

@cheatfate I just pushed the change and the tests now pass. Thank you!

Menduist reviewed Jan 31, 2023

View reviewed changes

Allow passing in trustAnchors to newTLSClientAsyncStream

4a81294

iffy force-pushed the custom-trust-anchors branch from c6eab3c to 4a81294 Compare February 1, 2023 15:16

Store X509TrustAnchors in a memory-secure, but memory-friendly way

a5dec78

Remove forgotten import

f46293f

cheatfate reviewed Feb 6, 2023

View reviewed changes

tests/testasyncstream.nim Outdated Show resolved Hide resolved

Change some waitFor to await in tests; add comment about trustAnchors…

eea849d

… reuse

cheatfate requested changes Feb 13, 2023

View reviewed changes

Remove use of result in newTrustAnchorStore

398da22

cheatfate merged commit 1b69b5e into status-im:master Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing in trustAnchors to newTLSClientAsyncStream #355

Allow passing in trustAnchors to newTLSClientAsyncStream #355

iffy commented Jan 30, 2023

Menduist commented Jan 31, 2023

Menduist Jan 31, 2023

cheatfate Jan 31, 2023

iffy Jan 31, 2023

Menduist Jan 31, 2023 •

edited

Loading

ringabout Feb 1, 2023

iffy Feb 1, 2023

cheatfate Feb 2, 2023

iffy commented Jan 31, 2023

iffy commented Feb 1, 2023

cheatfate commented Feb 2, 2023 •

edited

Loading

iffy commented Feb 2, 2023

cheatfate commented Feb 2, 2023 •

edited

Loading

iffy commented Feb 5, 2023 •

edited

Loading

cheatfate commented Feb 5, 2023 •

edited

Loading

iffy commented Feb 5, 2023

iffy commented Feb 5, 2023

cheatfate commented Feb 6, 2023

iffy commented Feb 6, 2023

iffy commented Feb 6, 2023

cheatfate commented Feb 6, 2023 •

edited

Loading

iffy commented Feb 6, 2023

cheatfate Feb 6, 2023 •

edited

Loading

iffy Feb 13, 2023

cheatfate commented Feb 17, 2023

cheatfate commented Feb 21, 2023

iffy commented Feb 21, 2023

Allow passing in trustAnchors to newTLSClientAsyncStream #355

Allow passing in trustAnchors to newTLSClientAsyncStream #355

Conversation

iffy commented Jan 30, 2023

Menduist commented Jan 31, 2023

Menduist Jan 31, 2023

Choose a reason for hiding this comment

cheatfate Jan 31, 2023

Choose a reason for hiding this comment

iffy Jan 31, 2023

Choose a reason for hiding this comment

Menduist Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

ringabout Feb 1, 2023

Choose a reason for hiding this comment

iffy Feb 1, 2023

Choose a reason for hiding this comment

cheatfate Feb 2, 2023

Choose a reason for hiding this comment

iffy commented Jan 31, 2023

iffy commented Feb 1, 2023

cheatfate commented Feb 2, 2023 • edited Loading

iffy commented Feb 2, 2023

cheatfate commented Feb 2, 2023 • edited Loading

iffy commented Feb 5, 2023 • edited Loading

cheatfate commented Feb 5, 2023 • edited Loading

iffy commented Feb 5, 2023

iffy commented Feb 5, 2023

cheatfate commented Feb 6, 2023

iffy commented Feb 6, 2023

iffy commented Feb 6, 2023

cheatfate commented Feb 6, 2023 • edited Loading

iffy commented Feb 6, 2023

cheatfate Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

iffy Feb 13, 2023

Choose a reason for hiding this comment

cheatfate commented Feb 17, 2023

cheatfate commented Feb 21, 2023

iffy commented Feb 21, 2023

Menduist Jan 31, 2023 •

edited

Loading

cheatfate commented Feb 2, 2023 •

edited

Loading

cheatfate commented Feb 2, 2023 •

edited

Loading

iffy commented Feb 5, 2023 •

edited

Loading

cheatfate commented Feb 5, 2023 •

edited

Loading

cheatfate commented Feb 6, 2023 •

edited

Loading

cheatfate Feb 6, 2023 •

edited

Loading