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

implement an rexec_client; move previous rexec code to rexec_server #36

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ env:
global:
- PINS="mirage-qubes.dev:. mirage-qubes-ipv4.dev:."
matrix:
- OCAML_VERSION=4.04 PACKAGE=mirage-qubes
- OCAML_VERSION=4.04 PACKAGE=mirage-qubes-ipv4
- OCAML_VERSION=4.05 PACKAGE=mirage-qubes
- OCAML_VERSION=4.05 PACKAGE=mirage-qubes-ipv4
- OCAML_VERSION=4.07 PACKAGE=mirage-qubes
- OCAML_VERSION=4.07 PACKAGE=mirage-qubes-ipv4
8 changes: 4 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@
.PHONY: build clean test

build:
jbuilder build @install --dev
dune build @install

test:
jbuilder runtest --dev
dune runtest

install:
jbuilder install
dune install

uninstall:
jbuilder uninstall
dune uninstall

clean:
rm -rf _build
Expand Down
26 changes: 26 additions & 0 deletions lib/formats.ml
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,19 @@ module Qrexec = struct
} [@@little_endian]
]

[%%cstruct
type trigger_service_params = {
service_name : uint8_t [@len 64];
target_domain : uint8_t [@len 32];
request_id : uint8_t [@len 32]
} [@@little_endian]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to preserve the these should be \0-terminated comments from the original header file?

]

type msg_type =
[ `Exec_cmdline
| `Just_exec
| `Service_connect
| `Service_refused
| `Trigger_service
| `Connection_terminated
| `Hello
Expand All @@ -57,6 +66,7 @@ module Qrexec = struct
| 0x200l -> `Exec_cmdline
| 0x201l -> `Just_exec
| 0x202l -> `Service_connect
| 0x203l -> `Service_refused
| 0x210l -> `Trigger_service
| 0x211l -> `Connection_terminated
| 0x300l -> `Hello
Expand All @@ -70,11 +80,27 @@ module Qrexec = struct
| `Exec_cmdline -> 0x200l
| `Just_exec -> 0x201l
| `Service_connect -> 0x202l
| `Service_refused -> 0x203l
| `Trigger_service -> 0x210l
| `Connection_terminated -> 0x211l
| `Hello -> 0x300l
| `Unknown x -> x

let string_of_type = function
| `Data_stdin -> "DATA_STDIN"
| `Data_stdout -> "DATA_STDOUT"
| `Data_stderr -> "DATA_STDERR"
| `Data_exit_code -> "DATA_EXIT_CODE"
| `Exec_cmdline -> "MSG_EXEC_CMDLINE"
| `Just_exec -> "MSG_JUST_EXEC"
| `Service_connect -> "MSG_SERVICE_CONNECT"
| `Service_refused -> "MSG_SERVICE_REFUSED"
| `Trigger_service -> "MSG_TRIGGER_SERVICE"
| `Connection_terminated -> "MSG_CONNECTION_TERMINATED"
| `Hello -> "MSG_HELLO"
| `Unknown x -> "Unknown message: " ^ (Int32.to_string x)


module Framing = struct
let header_size = sizeof_msg_header
let body_size_from_header h = get_msg_header_len h |> Int32.to_int
Expand Down
133 changes: 133 additions & 0 deletions lib/rExec_client.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
include RExec_common

open Lwt.Infix

let src = Logs.Src.create "qubes.rexec_client" ~doc:"Qubes qrexec-client"
module Log = (val Logs.src_log src : Logs.LOG)

module Flow = struct
type t = {
connection : QV.t;
mutable stderr_buf : Cstruct.t;
mutable stdout_buf : Cstruct.t;
}

let create connection = { connection; stderr_buf = Cstruct.empty;
stdout_buf = Cstruct.empty;
}

let write t data = send ~ty:`Data_stdin t.connection data

let writef t fmt =
fmt |> Printf.ksprintf @@ fun s ->
send ~ty:`Data_stdin t.connection (Cstruct.of_string (s ^ "\n"))

let next_msg t =
recv t.connection >|= function
| `Ok (`Data_stdout, data) ->
t.stdout_buf <- Cstruct.append t.stdout_buf data;
`Ok t
| `Ok (`Data_stderr, data) ->
t.stderr_buf <- Cstruct.append t.stderr_buf data;
`Ok t
| `Ok (`Data_exit_code, data) ->
`Done (Formats.Qrexec.get_exit_status_return_code data)
| `Ok (ty, _) ->
Log.debug Formats.Qrexec.(fun f -> f "unexpected message of type %ld (%s) received; \
ignoring it" (int_of_type ty) (string_of_type ty));
`Ok t
| `Eof -> `Eof

let read t =
let rec aux = function
| `Eof | `Done _ as s -> Lwt.return s
| `Ok t ->
let drain_stdout () =
let output = t.stdout_buf in
t.stdout_buf <- Cstruct.empty;
Lwt.return @@ `Stdout output
and drain_stderr () =
let output = t.stderr_buf in
t.stderr_buf <- Cstruct.empty;
Lwt.return @@ `Stderr output
in
if Cstruct.len t.stdout_buf > 0 then drain_stdout ()
else if Cstruct.len t.stderr_buf > 0 then drain_stderr ()
else next_msg t >>= aux
in
aux (`Ok t)

let rec read_line t =
let stdout = Cstruct.to_string t.stdout_buf
and stderr = Cstruct.to_string t.stderr_buf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to pose the cheeky question of whether converting both to string on each call was necessary, but to my surprise I found that Cstruct doesn't seem to have a index function. That sucks.

Perhaps we can do something like storing the length of the previously examined stdout_buf and stderr_buf so we can avoid checking the same bytes on long lines over and over?

I'd still prefer if we could avoid converting both buffers if we know we are always going to go with the first if there's a match; would it be OK with you to serialize these so the second conversion only happens when newline stdout is None?

in
let newline buf = String.index_opt buf '\n' in
match newline stdout, newline stderr with
| Some i, _ ->
let retval = String.sub stdout 0 i in
t.stdout_buf <- Cstruct.shift t.stdout_buf (i + 1);
Lwt.return (`Stdout retval)
| _, Some i ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is a problem, but as long as there's stdout_buf newlines we will never return anything from `Stderr.
It doesn't seem entirely fair, but I guess not an issue in real life.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to how read works FWIW. Stdout has priority over stderr.

let retval = String.sub stderr 0 i in
t.stderr_buf <- Cstruct.shift t.stderr_buf (i + 1);
Lwt.return (`Stderr retval)
| None, None ->
next_msg t >>= function
| `Done _ | `Eof as s -> Lwt.return s
| `Ok t -> read_line t
end

let start_connection data =
let domid = Formats.Qrexec.get_exec_params_connect_domain data in
let port = Formats.Qrexec.get_exec_params_connect_port data in
Log.debug (fun f -> f "service_connect message received: domain %ld, port %ld" domid port);
Log.debug (fun f -> f "Connecting...");
match Vchan.Port.of_string (Int32.to_string port) with
| `Error msg -> Lwt.return @@ Error (`Msg msg)
| `Ok port ->
QV.server ~domid:(Int32.to_int domid) ~port () >>= fun remote ->
send_hello remote >>= fun () ->
recv_hello remote >>= fun version ->
Log.debug (fun f -> f "server connected on port %s, using protocol version %ld" (Vchan.Port.to_string port) version);
Lwt.return @@ Ok (Flow.create remote)

let connect ~vm ~service ~identifier =
let write_trigger_service_parameters_into buf =
let write_string s ~dst_offset ~max_len =
Cstruct.blit_from_string s 0 buf dst_offset (min (String.length s) max_len)
in
write_string service ~dst_offset:0 ~max_len:64;
write_string vm ~dst_offset:64 ~max_len:32;
write_string identifier ~dst_offset:(64+32) ~max_len:32;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this doesn't use the [%struct] accessors functions like set_trigger_service_parameters_service_name? (Well that is a ridiculously long function name, but it does spare us the trouble of having to be careful to get the offsets and lengths right)

In the definition they seem to suggest these should be 0-terminated, so maybe we should pass max_len:whatever-1, or validate + fail if the application decides to pass us garbage input?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in QubesOS does add a zero byte at the very end of the char array in any case. But I agree that we should ensure it's also zero-terminated on our end because otherwise we potentially lose one byte of the identifier. https://github.com/QubesOS/qubes-core-qrexec/blob/master/daemon/qrexec-daemon.c#L817

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed in qrexec-client-vm that they just pass SOCKET every time as the identifier. Maybe we should do the same and remove identifier from the arguments making it simpler.

buf
in
let tsp = write_trigger_service_parameters_into @@
Cstruct.create Formats.Qrexec.sizeof_trigger_service_params in
Log.debug (fun f -> f "Initiating connection to dom0 (to request service start)");
QV.server ~domid:0 ~port:vchan_base_port () >>= fun server ->
RExec_common.send_hello server >>= fun () ->
RExec_common.recv_hello server >>= fun version ->
Log.debug (fun f -> f "connection with dom0 established (version %ld)" version);
RExec_common.send server ~ty:`Trigger_service tsp >>= function
| `Eof -> Lwt.return @@ Error `Closed
| `Ok () ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XXX Not related to this PR, but I went looking for the receiving side of this, and fell over qubes-core-agent-linux/qrexec/qrexec-agent.c:handle_server_exec_request which seems to assume that buf is 0-terminated:

    if ((hdr->type == MSG_EXEC_CMDLINE || hdr->type == MSG_JUST_EXEC) &&
            !strstr(buf, ":nogui:")) {
   /* seems to me this strstr() should probably be a memmem() bounded to buf_len */

let rec try_recv () =
recv server >>= function
| `Eof -> Lwt.return @@ Error `Closed
| `Ok (`Service_refused, _) -> Lwt.return @@ Error `Permission_denied
| `Ok (`Service_connect, data) ->
(* we have everything we need, so close the server connection *)
QV.disconnect server >>= fun () ->
start_connection data
| `Ok (ty, _) ->
let open Formats.Qrexec in
Log.debug (fun f -> f
"unhandled qrexec message type received in response to \
trigger service request: %ld (%s)"
(int_of_type ty) (string_of_type ty));
try_recv ()
in
try_recv ()

let close t =
QV.disconnect t.Flow.connection
27 changes: 27 additions & 0 deletions lib/rExec_client.mli
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
module Flow : sig
type t

val write : t -> Cstruct.t -> [ `Ok of unit | `Eof ] Lwt.t

val writef : t -> ('a, unit, string, unit S.or_eof Lwt.t) format4 -> 'a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like documenting the trailing \n being added here would make sense, if we want to keep it.

It seems confusing to me that write would not add a newline, while writef does.
This behavior is in line with the [current implementation of RExec.writef]https://github.com/mirage/mirage-qubes/blob/master/lib/rExec.ml#L66-L83) which is documented in the mli interface (which I also find confusing). :)


val read : t -> [ `Stderr of Cstruct.t
| `Stdout of Cstruct.t
| `Done of int32 | `Eof ] Lwt.t

val read_line : t -> [ `Stderr of string
| `Stdout of string
| `Done of int32 | `Eof ] Lwt.t

end

val connect : vm:string -> service:string -> identifier:string ->
(Flow.t, [`Closed | `Permission_denied | `Msg of string ]) result Lwt.t
(** Attempt to establish a qrexec connection to the guest named [vm],
and try to start the provided [service].
Use [identifier] to disambiguate this traffic.
*)

val close : Flow.t -> unit Lwt.t
(** Close the underlying vchan without waiting for the remote side to complete.
Any remaining messages will be discarded. *)
67 changes: 67 additions & 0 deletions lib/rExec_common.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
open Formats.Qrexec
open Utils
open Lwt.Infix

module QV = Msg_chan.Make(Framing)

type t = QV.t

let (>>!=) = Msg_chan.(>>!=)

let split chr s =
try
let i = String.index s chr in
Some (String.sub s 0 i, String.sub s (i + 1) (String.length s - i - 1))
with Not_found ->
None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could avoid exceptions with something like:

let split chr s =
  match String.index_opt s chr with
  | None -> None
  | Some i -> Some (String.sub s 0 i, String.sub s (i + 1) (String.length s - i - 1))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String.index_opt was added in 4.05.0 and that's our new minimum, so I concur.


let or_fail = function
| `Ok y -> return y
| `Error (`Unknown msg) -> fail (Failure msg)
| `Eof -> fail End_of_file

let disconnect = QV.disconnect

let vchan_base_port =
match Vchan.Port.of_string "512" with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this constant come from?

| `Error msg -> failwith msg
| `Ok port -> port

let max_data_chunk = 4096
(** Max size for data chunks. See MAX_DATA_CHUNK in qubes-linux-utils/qrexec-lib/qrexec.h *)

let rec send t ~ty data =
let data, data' = Cstruct.split data (min max_data_chunk (Cstruct.len data)) in
let hdr = Cstruct.create sizeof_msg_header in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we wrapped the body in a recursive function we could avoid an allocation of Cstruct.create sizeof_msg_header by reusing it (necessitating only the set_msg_header_len hdr (Cstruct.len data |> Int32.of_int); in the loop).
Since max_data_chunk = 4096 this might make sense for large sends like file transfers?

set_msg_header_ty hdr (int_of_type ty);
set_msg_header_len hdr (Cstruct.len data |> Int32.of_int);
if Cstruct.len data' = 0
then QV.send t [hdr; data]
else QV.send t [hdr; data] >>= function
| `Eof -> return `Eof
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to return `Eof when we haven't sent all data?

| `Ok () ->
send t ~ty data'

let recv t =
QV.recv t >>!= fun (hdr, data) ->
let ty = get_msg_header_ty hdr |> type_of_int in
return (`Ok (ty, data))

let send_hello t =
let hello = Cstruct.create sizeof_peer_info in
set_peer_info_version hello 2l;
send t ~ty:`Hello hello >>= function
| `Eof -> fail (error "End-of-file sending msg_hello")
| `Ok () -> return ()

let recv_hello t =
recv t >>= function
| `Eof -> fail (error "End-of-file waiting for msg_hello")
| `Ok (`Hello, resp) -> return (get_peer_info_version resp)
| `Ok (ty, _) -> fail (error "Expected msg_hello, got %ld" (int_of_type ty))

let port_of_int i =
match Int32.to_string i |> Vchan.Port.of_string with
| `Ok p -> p
| `Error msg -> failwith msg