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

Jump to bottom

null bytes truncate filenames given to Sys and Unix #6945

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

null bytes truncate filenames given to Sys and Unix #6945

vicuna opened this issue Jul 30, 2015 · 11 comments
Labels
bug newcomer-job stdlib

Comments

@vicuna
Copy link

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

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

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

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

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

vicuna commented Aug 5, 2015

Comment author: @c-cube

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

@vicuna
Copy link
Author

vicuna commented Nov 11, 2015

Comment author: @xavierleroy

Fixed in trunk by commits dc043a7 and 9dfa69e.

@vicuna
Copy link
Author

vicuna commented Nov 13, 2015

Comment author: @dbuenzli

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

#227 (comment)

1 similar comment
@vicuna
Copy link
Author

vicuna commented Nov 13, 2015

Comment author: @dbuenzli

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

#227 (comment)

@vicuna
Copy link
Author

vicuna commented Nov 13, 2015

Comment author: @xavierleroy

Missing checks added in commits 29a50b4 and dc2a98c

@vicuna
Copy link
Author

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

vicuna commented Nov 15, 2015

Comment author: @xavierleroy

More missing checks added in commit 9893e26.

@vicuna vicuna closed this as completed Feb 16, 2017
@gasche gasche mentioned this issue Jun 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug newcomer-job stdlib
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@vicuna