Re: [PATCH v2 01/23] iotests: Introduce $SOCK_DIR

[Max Reitz]

On 17.10.19 16:52, Eric Blake wrote:
> On 10/17/19 8:31 AM, Max Reitz wrote:
>> Unix sockets generally have a maximum path length.  Depending on your
>> $TEST_DIR, it may be exceeded and then all tests that create and use
>> Unix sockets there may fail.
>>
>> Circumvent this by adding a new scratch directory specifically for
>> Unix socket files.  It defaults to a temporary directory (mktemp -d)
>> that is completely removed after the iotests are done.
>>
>> (By default, mktemp -d creates a /tmp/tmp.XXXXXXXXXX directory, which
>> should be short enough for our use cases.)
>>
>> Use mkdir -p to create the directory (because it seems right), and do
>> the same for $TEST_DIR (because there is no reason for that to be
>> created in any different way).
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>   tests/qemu-iotests/check | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
>> @@ -116,10 +117,14 @@ set_prog_path()
>>   if [ -z "$TEST_DIR" ]; then
>>           TEST_DIR=$PWD/scratch
>>   fi
>> +mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'
> 
> This one seems fine. We are either using the user's name (and if it is
> pre-existing, not fail) or using a well-known name (if someone else
> slams in files into that directory in parallel with our test run, oh
> well).  But at least the well-known name is a directory that is probably
> already accessible only to the current user, not world-writable.
> 
>>   -if [ ! -e "$TEST_DIR" ]; then
>> -        mkdir "$TEST_DIR"
>> +tmp_sock_dir=false
>> +if [ -z "$SOCK_DIR" ]; then
>> +    SOCK_DIR=$(mktemp -d)
>> +    tmp_sock_dir=true
>>   fi
>> +mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
> 
> Thinking about this again: if the user passed in a name, we probably
> want to use it no matter whether the directory already exists (mkdir -p
> makes sense: either the directory did not exist, or the user is in
> charge of passing us a directory that they already secured).  But if we
> generate our own name in a world-writable location in /tmp, using mkdir
> -p means someone else can race us to the creation of the directory, and
> potentially populate it in a way to cause us a security hole while we
> execute our tests.

I don’t quite see how this is a security hole.  mktemp -d creates the
directory, so noone can race us.

Max

> I would be a bit more comfortable with:
> 
> tmp_sock_dir=false
> tmp_sock_opt=-p
> if [ -z "$SOCK_DIR" ]; then
>     SOCK_DIR=$(mktemp -d)
>     tmp_sock_dir=true
>     tmp_sock_opt=      # disable -p for our generated name
> fi
> mkdir $tmp_sock_opt "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
>

signature.asc
Description: OpenPGP digital signature