|
|
Created:
4 years, 10 months ago by Sean Klein Modified:
4 years, 10 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionExtended restricted filesystem to support relative paths.
Usage of ".." is still disallowed.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=4152
TESTS=limited_file_access_test
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/9b1c913840204c4794106c2a6db0068e5be5377a
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Using strcat instead of memcpy #
Total comments: 4
Patch Set 4 : Allow relative paths without prepending CWD #
Total comments: 8
Patch Set 5 : Strcpy, Testing cwd #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 22 (6 generated)
smklein@chromium.org changed reviewers: + sbc@chromium.org
Thanks for doing this. I think I can live without '..' for not but it would be nice to have that work too eventually. https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:73: return -NaClXlateErrno(errno); NaClHostDescGetcwd returns the translated errno already so I would simply return whatever it returns. https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:100: absolute_path[cwd_path_len + 1 + virtual_path_len] = '\0'; Is there some reason you are not using strcat here which would auto-terminate? All these stings are NULL terminated right? and we know we don't overflow because of the check above, right?
smklein@chromium.org changed reviewers: + mseaborn@chromium.org
Adding Mark. I can look into adding ".." support in an upcoming CL. https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:73: return -NaClXlateErrno(errno); On 2016/02/11 22:45:26, Sam Clegg wrote: > NaClHostDescGetcwd returns the translated errno already so I would simply return > whatever it returns. Done. https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:100: absolute_path[cwd_path_len + 1 + virtual_path_len] = '\0'; On 2016/02/11 22:45:26, Sam Clegg wrote: > Is there some reason you are not using strcat here which would auto-terminate? Well, strcat is slower, since it requires scanning the intermediate string linearly to determine the length before concatenating. > All these stings are NULL terminated right? and we know we don't overflow > because of the check above, right? The intermediate steps do not require that absolute_path is NULL-terminated.
On 2016/02/11 23:00:52, smklein1 wrote: > Adding Mark. > > I can look into adding ".." support in an upcoming CL. > > https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... > File src/trusted/service_runtime/sel_ldr_filename.c (right): > > https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... > src/trusted/service_runtime/sel_ldr_filename.c:73: return > -NaClXlateErrno(errno); > On 2016/02/11 22:45:26, Sam Clegg wrote: > > NaClHostDescGetcwd returns the translated errno already so I would simply > return > > whatever it returns. > > Done. > > https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... > src/trusted/service_runtime/sel_ldr_filename.c:100: absolute_path[cwd_path_len + > 1 + virtual_path_len] = '\0'; > On 2016/02/11 22:45:26, Sam Clegg wrote: > > Is there some reason you are not using strcat here which would auto-terminate? > > > Well, strcat is slower, since it requires scanning the intermediate string > linearly to determine the length before concatenating. > > > All these stings are NULL terminated right? and we know we don't overflow > > because of the check above, right? > > The intermediate steps do not require that absolute_path is NULL-terminated. I think I'd find it easier to read in terms of strcat. Maybe there are some other subtle issues, but I'm not convinced the optimization is worth here, especially since all the callers will be making filesystem-related syscalls anyway.
On 2016/02/11 23:04:05, Sam Clegg wrote: > On 2016/02/11 23:00:52, smklein1 wrote: > > Adding Mark. > > > > I can look into adding ".." support in an upcoming CL. > > > > > https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... > > File src/trusted/service_runtime/sel_ldr_filename.c (right): > > > > > https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... > > src/trusted/service_runtime/sel_ldr_filename.c:73: return > > -NaClXlateErrno(errno); > > On 2016/02/11 22:45:26, Sam Clegg wrote: > > > NaClHostDescGetcwd returns the translated errno already so I would simply > > return > > > whatever it returns. > > > > Done. > > > > > https://codereview.chromium.org/1690983004/diff/20001/src/trusted/service_run... > > src/trusted/service_runtime/sel_ldr_filename.c:100: absolute_path[cwd_path_len > + > > 1 + virtual_path_len] = '\0'; > > On 2016/02/11 22:45:26, Sam Clegg wrote: > > > Is there some reason you are not using strcat here which would > auto-terminate? > > > > > > Well, strcat is slower, since it requires scanning the intermediate string > > linearly to determine the length before concatenating. > > > > > All these stings are NULL terminated right? and we know we don't overflow > > > because of the check above, right? > > > > The intermediate steps do not require that absolute_path is NULL-terminated. > > I think I'd find it easier to read in terms of strcat. Maybe there are some > other subtle issues, but I'm not convinced the optimization is worth here, > especially since all the callers will be making filesystem-related syscalls > anyway. Updated to use strcat instead of memcpy.
Description was changed from ========== Extended restricted filesystem to support relative paths. NOTE: ".." still disallowed. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4152 TESTS=limited_file_access_test ========== to ========== Extended restricted filesystem to support relative paths. Usage of ".." is still disallowed. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4152 TESTS=limited_file_access_test ==========
https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:69: /* Absolute Path = Cwd + '/' + Relative Virtual Path + '\0' */ You don't actually need to do this concatenation. You can just use the relative path directly. That would be equivalent and simpler -- there's less to go wrong. The only difference is that then the ValidateAbsolutePath() check won't pass. But that check is redundant -- how about removing it or making it conditional?
https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:69: /* Absolute Path = Cwd + '/' + Relative Virtual Path + '\0' */ Although I know it's technically not necessary now, conversion from relative to absolute paths will be once ".." is added. As you mentioned in a previous code review, race conditions would be possible with a) Relative paths containing "..", and b) Background threads changing the PWD. This string concatenation will need to be done either now or later. Would you prefer I cut it out of this CL and paste it into the next one? (personally I would prefer for these changes to be done more incrementally, but it's your call! I can remove it for now.)
https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:69: /* Absolute Path = Cwd + '/' + Relative Virtual Path + '\0' */ On 2016/02/17 22:42:39, Sean Klein wrote: > Although I know it's technically not necessary now, conversion from relative to > absolute paths will be once ".." is added. As you mentioned in a previous code > review, race conditions would be possible with > a) Relative paths containing "..", and > b) Background threads changing the PWD. So you plan to handle paths containing "..", but without allowing symlinks? > This string concatenation will need to be done either now or later. Would you > prefer I cut it out of this CL and paste it into the next one? I'd prefer to cut it out of this CL. I'd prefer to review it only when the need for it is made apparent by other logic that really needs it. :-) Also, as I've mentioned before, I really don't want to be in the business of reviewing C code for manipulating strings in fixed-length buffers, when we could just change this to a .cc file and use C++'s std::string instead. Adding a one-off piece of C string manipulation code is one thing, but incrementally adding more is a sign that we should really be using something safer.
https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/40001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:69: /* Absolute Path = Cwd + '/' + Relative Virtual Path + '\0' */ On 2016/02/18 19:32:02, Mark Seaborn wrote: > On 2016/02/17 22:42:39, Sean Klein wrote: > > Although I know it's technically not necessary now, conversion from relative > to > > absolute paths will be once ".." is added. As you mentioned in a previous code > > review, race conditions would be possible with > > a) Relative paths containing "..", and > > b) Background threads changing the PWD. > > So you plan to handle paths containing "..", but without allowing symlinks? That is the short-term plan, yes. > > This string concatenation will need to be done either now or later. Would you > > prefer I cut it out of this CL and paste it into the next one? > > I'd prefer to cut it out of this CL. I'd prefer to review it only when the need > for it is made apparent by other logic that really needs it. :-) > > Also, as I've mentioned before, I really don't want to be in the business of > reviewing C code for manipulating strings in fixed-length buffers, when we could > just change this to a .cc file and use C++'s std::string instead. > > Adding a one-off piece of C string manipulation code is one thing, but > incrementally adding more is a sign that we should really be using something > safer. Got it! I'll leave it out. I'll start working on translating this file to C++ in a separate CL.
Mark, could you take a look? I've simplified the relative path case.
https://codereview.chromium.org/1690983004/diff/60001/documentation/filesyste... File documentation/filesystem_access.txt (right): https://codereview.chromium.org/1690983004/diff/60001/documentation/filesyste... documentation/filesystem_access.txt:70: Requires that the cwd is within the mounted directory (set at initialization). Make this "Path sanitization checks that..." so that this is a full sentence. https://codereview.chromium.org/1690983004/diff/60001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/60001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:56: real_path[0] = '\0'; /* Required to strcat from start of path. */ Why not just use strcpy() instead of the first calls to strcat()? https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... File tests/limited_file_access/limited_file_access.cc (right): https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... tests/limited_file_access/limited_file_access.cc:6: If I comment out the call to NaClHostDescChdir() in sel_main.c, use of relative paths becomes unsafe, but the test still passes. Can you make sure the test checks that the initial cwd is safe?
https://codereview.chromium.org/1690983004/diff/60001/documentation/filesyste... File documentation/filesystem_access.txt (right): https://codereview.chromium.org/1690983004/diff/60001/documentation/filesyste... documentation/filesystem_access.txt:70: Requires that the cwd is within the mounted directory (set at initialization). On 2016/02/24 21:19:19, Mark Seaborn wrote: > Make this "Path sanitization checks that..." so that this is a full sentence. Done. https://codereview.chromium.org/1690983004/diff/60001/src/trusted/service_run... File src/trusted/service_runtime/sel_ldr_filename.c (right): https://codereview.chromium.org/1690983004/diff/60001/src/trusted/service_run... src/trusted/service_runtime/sel_ldr_filename.c:56: real_path[0] = '\0'; /* Required to strcat from start of path. */ On 2016/02/24 21:19:19, Mark Seaborn wrote: > Why not just use strcpy() instead of the first calls to strcat()? Done. https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... File tests/limited_file_access/limited_file_access.cc (right): https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... tests/limited_file_access/limited_file_access.cc:6: Updated the first test to verify the cwd before calling "chdir". This should catch a missing NaClHostDescChdir.
LGTM https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... File tests/limited_file_access/limited_file_access.cc (right): https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... tests/limited_file_access/limited_file_access.cc:6: On 2016/02/24 23:40:43, Sean Klein wrote: > Updated the first test to verify the cwd before calling "chdir". This should > catch a missing NaClHostDescChdir. Can you also add a comment to that code in sel_main.c, e.g. "This is required for safety, because we allow relative pathnames." https://codereview.chromium.org/1690983004/diff/80001/tests/limited_file_acce... File tests/limited_file_access/limited_file_access.cc (right): https://codereview.chromium.org/1690983004/diff/80001/tests/limited_file_acce... tests/limited_file_access/limited_file_access.cc:341: do_test_write_read_file(file_name, new_file); It would be clearer to write "/* new_file= */ false", so that it's more obviously a constant.
https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... File tests/limited_file_access/limited_file_access.cc (right): https://codereview.chromium.org/1690983004/diff/60001/tests/limited_file_acce... tests/limited_file_access/limited_file_access.cc:6: On 2016/02/25 01:18:09, Mark Seaborn wrote: > On 2016/02/24 23:40:43, Sean Klein wrote: > > Updated the first test to verify the cwd before calling "chdir". This should > > catch a missing NaClHostDescChdir. > > Can you also add a comment to that code in sel_main.c, e.g. "This is required > for safety, because we allow relative pathnames." Done. https://codereview.chromium.org/1690983004/diff/80001/tests/limited_file_acce... File tests/limited_file_access/limited_file_access.cc (right): https://codereview.chromium.org/1690983004/diff/80001/tests/limited_file_acce... tests/limited_file_access/limited_file_access.cc:341: do_test_write_read_file(file_name, new_file); On 2016/02/25 01:18:09, Mark Seaborn wrote: > It would be clearer to write "/* new_file= */ false", so that it's more > obviously a constant. Done.
The CQ bit was checked by smklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org Link to the patchset: https://codereview.chromium.org/1690983004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690983004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690983004/100001
Message was sent while issue was closed.
Description was changed from ========== Extended restricted filesystem to support relative paths. Usage of ".." is still disallowed. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4152 TESTS=limited_file_access_test ========== to ========== Extended restricted filesystem to support relative paths. Usage of ".." is still disallowed. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4152 TESTS=limited_file_access_test Committed: https://chromium.googlesource.com/native_client/src/native_client/+/9b1c91384... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/native_client/src/native_client/+/9b1c91384... |