ulrikstrid / ocaml-jose

https://ulrikstrid.github.io/ocaml-jose/
MIT License
53 stars 4 forks source link

`Es256_priv` Jwk always fails `validate_signature` with `Invalid_signature` #63

Closed dmmulroy closed 1 year ago

dmmulroy commented 1 year ago

I can't seem to properly sign and validate a JWT using a ES256 private key.

Steps to reproduce:

  1. Generate new ES256 private key using: openssl ecparam -genkey -name prime256v1 -noout -out private_key.pem
  2. Create a new Es256_priv Jwk.t using Jwk.of_priv_pem and paste in the above private key as the string argument
  3. Create a Header.t via Header.make_header and using the created jwk
  4. Create a jwt let jwt = Jwt.sign ~header ~payload:Jwt.empty_payload jwk |> Result.get_ok;;
  5. Validate signature: Jwt.validate_signature ~jwk jwt;;
- : (Jwt.t, [> `Invalid_signature | `Msg of string ]) result =
Error `Invalid_signature
dmmulroy commented 1 year ago

It also appears that there are several failing tests around the ECDSA implementations - I tested with compiler versions 5.0.0 and 4.14.1

dmmulroy commented 1 year ago

image

anmonteiro commented 1 year ago

@dmmulroy looking into this, but tests are passing: https://github.com/ulrikstrid/ocaml-jose/actions/runs/5646938425/job/15295950081?pr=64

anmonteiro commented 1 year ago

@dmmulroy regarding the example in your image, even if the encoding is different, the key is the same. You can check that the bytes are exactly the same with:

$ openssl pkey -in private_key.pem -text
anmonteiro commented 1 year ago

@dmmulroy FWIW, this test program works for me. Could you give more details as to which versions you're running?

open Jose

let read_file filename =
  let ic = open_in filename in
  let n = in_channel_length ic in
  let s = really_input_string ic n in
  close_in ic;
  s

let test () =
  let priv_pem = read_file "private_key.pem" in
  let jwk = Jwk.of_priv_pem priv_pem |> Result.get_ok in
  let header = Header.make_header jwk in
  let jwt = Jwt.sign ~header ~payload:Jwt.empty_payload jwk |> Result.get_ok in
  match Jwt.validate_signature ~jwk jwt with
  | Ok _jwt -> Format.eprintf "success@."
  | Error `Invalid_signature -> Format.eprintf "ERR(0): Invalid Signature@."
  | Error (`Msg msg) -> Format.eprintf "ERR(1): %s@." msg

let () = test ()
ghost commented 1 year ago

Let me see if I can make a repo where it's reproducible - thanks for the quick help!

dmmulroy commented 1 year ago

Okay @anmonteiro - here's a repo with a opam lock file and a test private key that is causing the issue for me.

https://github.com/dmmulroy/jose-jwk-test

~/Code/jose_test on  main via 🐫 v4.14.1 (*jose_test) 
> dune exec bin/main.exe  
ERR(0): Invalid Signature   
anmonteiro commented 1 year ago

On a cursory look, it looks like we're using the same versions. I'll take a closer look later.

dmmulroy commented 1 year ago

Small update - I think I have the problem narrowed down to Mirage_crypto_ec.P256.Dsa.pub_to_cstruct but not too sure on how to proceed.

Alcotest.test_case "Mirage_crypto_ec.P256.Dsa cstruct test" `Quick
  (fun () ->
    let open Jose.Utils in
    let x = "q3zAwR_kUwtdLEwtB2oVfucXiLHmEhu9bJUFYjJxYGs" in
    let y = "8h0D-ONoU-iZqrq28TyUxEULxuGwJZGMJYTMbeMshvI" in

    let make_ES256_of_x_y (x, y) =
      let x =
        U_Base64.url_decode x |> U_Result.map Cstruct.of_string
      in
      let y =
        U_Base64.url_decode y |> U_Result.map Cstruct.of_string
      in
      U_Result.both x y
      |> U_Result.map (fun (x, y) ->
             let four = Cstruct.create 1 in
             Cstruct.set_uint8 four 0 4;
             let point = Cstruct.concat [ four; x; y ] in
             let k = Mirage_crypto_ec.P256.Dsa.pub_of_cstruct point in
             k |> U_Result.get_exn)
    in

    let get_ES256_x_y key =
      let point = Mirage_crypto_ec.P256.Dsa.pub_to_cstruct key in
      let x_cs, y_cs = Cstruct.(split (shift point 1) 32) in
      let x =
        x_cs |> Cstruct.to_string |> U_Base64.url_encode_string
      in
      let y =
        y_cs |> Cstruct.to_string |> U_Base64.url_encode_string
      in
      (x, y)
    in
    let pub_key = make_ES256_of_x_y (x, y) |> Result.get_ok in
    let x', y' = get_ES256_x_y pub_key in

    check_result_string "x" (Ok x) (Ok x');
    check_result_string "y" (Ok y) (Ok y'));
FAIL x
   Expected: `Ok "q3zAwR_kUwtdLEwtB2oVfucXiLHmEhu9bJUFYjJxYGs"'
   Received: `Ok "4H8xy6EO7QkUjjdjwkmxlM-pw2lWbMZx3wja3-HM6mc"'
dmmulroy commented 1 year ago

and further:

Alcotest.test_case "Mirage_crypto_ec.P256.Dsa cstruct test 2" `Quick
  (fun () ->
    let open Jose.Utils in
    let x = "q3zAwR_kUwtdLEwtB2oVfucXiLHmEhu9bJUFYjJxYGs" in
    let y = "8h0D-ONoU-iZqrq28TyUxEULxuGwJZGMJYTMbeMshvI" in

    let make_cstruct_point_of_x_y (x, y) =
      let x =
        U_Base64.url_decode x |> U_Result.map Cstruct.of_string
      in
      let y =
        U_Base64.url_decode y |> U_Result.map Cstruct.of_string
      in
      U_Result.both x y
      |> U_Result.map (fun (x, y) ->
             let four = Cstruct.create 1 in
             Cstruct.set_uint8 four 0 4;
             Cstruct.concat [ four; x; y ])
    in

    let get_cstruct_point_x_y key =
      Mirage_crypto_ec.P256.Dsa.pub_to_cstruct key
    in

    let cstruct_point =
      Result.get_ok @@ make_cstruct_point_of_x_y (x, y)
    in
    let key =
      Result.get_ok
      @@ Mirage_crypto_ec.P256.Dsa.pub_of_cstruct cstruct_point
    in
    let cstruct_point' = get_cstruct_point_x_y key in

    check_result_string "cstruct point"
      (Ok (Cstruct.to_string cstruct_point))
      (Ok (Cstruct.to_string cstruct_point')));
FAIL cstruct point

   Expected: `Ok
                "\004\171|\192\193\031\228S\011],L-\007j\021~\231\023\136\177\230\018\027\189l\149\005b2q`k\242\029\003\248\227hS\232\153\170\186\182\241<\148\196E\011\198\225\176%\145\140%\132\204m\227,\134\242"'

   Received: `Ok
                "\004\224\1271\203\161\014\237\t\020\1427c\194I\177\148\207\169\195iVl\198q\223\b\218\223\225\204\234g\147^uHu\208|=Z\168\157\225\252\181 \205\211\142I\241@\248\178\184\154\127<|\011:\239$"'
anmonteiro commented 1 year ago

I haven’t confirmed your findings but if you suspect a bug in mirage crypto then we should report that in that repo

ghost commented 1 year ago

This is a bug in clang 14.0.3 and Apple hasn't patched it yet. See: https://github.com/mit-plv/fiat-crypto/issues/1606

anmonteiro commented 1 year ago

FTR that explains why I couldn't repro:

$ clang --version
clang version 11.1.0
Target: aarch64-apple-darwin
Thread model: posix
InstalledDir: /nix/store/gcbmg7a0fhdcda42g7qp7ngqcix7bax5-clang-11.1.0/bin
hannesm commented 1 year ago

FWIW, mirage-crypto 0.11.2 avoids the miscompilation (PRed to opam-repository https://github.com/ocaml/opam-repository/pull/24461) -- sorry it took so long to get this out of the door.