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

null bytes truncate filenames given to Sys and Unix #6945

Closed
vicuna opened this issue Jul 30, 2015 · 11 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Jul 30, 2015

Original bug ID: 6945
Reporter: @dbuenzli
Status: closed (set by @xavierleroy on 2017-02-16T14:14:34Z)
Resolution: fixed
Priority: normal
Severity: major
Version: 4.02.3
Fixed in version: 4.03.0+dev / +beta1
Category: standard library
Tags: junior_job
Monitored by: @gasche @diml @hcarty @dbuenzli

Bug description

Hello,

Functions of the Sys and Unix module taking filenames as arguments simply truncate their argument on null bytes. They should rather raise Sys_error and Unix.ENOENT, I'm sure there are all kinds of nice security exploits to perform with the current scheme, see e.g. http://projects.webappsec.org/w/page/13246949/Null%20Byte%20Injection

Best,

Daniel

Steps to reproduce

mkdir -p /tmp/bla
ocaml
OCaml version 4.02.3

Sys.file_exists "/tmp/bla\x00hehey";;

  • : bool = true

#require "unix";;

/Users/dbuenzli/.opam/4.02.3/lib/ocaml/unix.cma: loaded

Unix.stat "/tmp/bla\x00hehey";;

  • : Unix.stats =
    {Unix.st_dev = 16777220; st_ino = 113544695; st_kind = Unix.S_DIR;
    st_perm = 493; st_nlink = 2; st_uid = 501; st_gid = 0; st_rdev = 0;
    st_size = 68; st_atime = 1438290718.; st_mtime = 1438290111.;
    st_ctime = 1438290111.}
@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Comment author: @gasche

A first idea for a fix would be to assert, in each of the C functions that passes an OCaml string to the system, that the strlen is equal to the OCaml length of the string (this should be a relatively fast test). Is that a reasonable way to proceed?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Comment author: @diml

Note that for Unix.connect and Unix.bind you need to allow the first byte of the path to be null as it is used for abstract socket on linux.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Comment author: @dbuenzli

@gasche that seems reasonable. W.r.t. what @dim says, the test should simply not be done on C functions that take a length argument specifying the passed string length.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Comment author: @diml

Well, in case of connect or bind for instance it make sense to still do the test if the first byte is not null, as in this case they will ignore everything after the null terminating byte.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 5, 2015

Comment author: @c-cube

a fix for the functions mentioned in the ticket: #227

@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 11, 2015

Comment author: @xavierleroy

Fixed in trunk by commits dc043a7 and 9dfa69e.

@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 13, 2015

Comment author: @dbuenzli

Reopening this before I forget. The fix is not complete see

#227 (comment)

1 similar comment
@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 13, 2015

Comment author: @dbuenzli

Reopening this before I forget. The fix is not complete see

#227 (comment)

@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 13, 2015

Comment author: @xavierleroy

Missing checks added in commits 29a50b4 and dc2a98c

@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 13, 2015

Comment author: @dbuenzli

Made a quick review of the OCaml C system interface we are still missing a few ones. See

#227 (comment)

@vicuna

This comment has been minimized.

Copy link
Author

commented Nov 15, 2015

Comment author: @xavierleroy

More missing checks added in commit 9893e26.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.