Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Es256_priv Jwk always fails validate_signature with Invalid_signature #63

Closed
dmmulroy opened this issue Jul 24, 2023 · 14 comments
Closed

Comments

@dmmulroy
Copy link

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 dmmulroy changed the title Es256_priv Jwk always fails validate_signature with Invalid_signature Es256_priv Jwk always fails validate_signature with `Invalid_signature Jul 24, 2023
@dmmulroy dmmulroy changed the title Es256_priv Jwk always fails validate_signature with `Invalid_signature Es256_priv Jwk always fails validate_signature with \Invalid_signature` Jul 24, 2023
@dmmulroy dmmulroy changed the title Es256_priv Jwk always fails validate_signature with \Invalid_signature` Es256_priv Jwk always fails validate_signature with Invalid_signature Jul 24, 2023
@dmmulroy
Copy link
Author

dmmulroy commented Jul 24, 2023

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
Copy link
Author

image

@anmonteiro
Copy link
Collaborator

@anmonteiro
Copy link
Collaborator

@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
Copy link
Collaborator

anmonteiro commented Jul 24, 2023

@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
Copy link

ghost commented Jul 24, 2023

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

@dmmulroy
Copy link
Author

dmmulroy commented Jul 24, 2023

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
Copy link
Collaborator

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

@dmmulroy
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

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

@ghost
Copy link

ghost commented Jul 26, 2023

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

@anmonteiro
Copy link
Collaborator

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 added a commit to hannesm/opam-repository that referenced this issue Sep 18, 2023
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.2)

CHANGES:

* mirage-crypto-rng-eio: improve portability by using eio 0.7's monotonic clock
  interface instead of mtime.clock.os. (mirage/mirage-crypto#176 @TheLortex)
* mirage-crypto-rng-eio: update to eio 0.12 (mirage/mirage-crypto#182 @talex5)
* mirage-crypto-rng: fix typo in RNG setup (mirage/mirage-crypto#179 @samueldurantes)
* macOS: on arm64 with clang 14.0.3, avoid instcombine (due to miscompilations)
  reported by @samoht mit-plv/fiat-crypto#1606 (comment)
  re-reported in ulrikstrid/ocaml-jose#63 and mirleft/ocaml-tls#478
  (mirage/mirage-crypto#185 @hannesm @kit-ty-kate)
* avoid "stringop-overflow" warning on PPC64 and S390x (spurious warnings) when
  in devel mode (mirage/mirage-crypto#178 mirage/mirage-crypto#184 @avsm @hannesm)
* stricter C prototypes, unsigned/signed integers (mirage/mirage-crypto#175 @MisterDA @haesbaert
  @avsm @hannesm)
* support DragonFlyBSD (mirage/mirage-crypto#181 @movepointsolutions)
* support GNU/Hurd (mirage/mirage-crypto#174 @pinotree)
@hannesm
Copy link
Contributor

hannesm commented Sep 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants