Opened 7 years ago

Closed 7 years ago

#2595 closed defect (fixed)

cmdctl with chusr crashes due to permission issue

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20130205
Component: ~cmd-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 5.25 Internal?: no

Description

Assume you install BIND 10 as a super user, start bind10 with "-u someone"
("someone" doesn't have full root privilege), and assume that the
permission issues for the lock file and UNIX domain socket for msgq are
resolved (see #711). If you try to control it via bindctl, it fails
as follows:

% /usr/local/bin/bindctl
Socket error while sending login information: [Errno 8] _ssl.c:392: EOF occurred in violation of protocol
[1]    88494 exit 1     /usr/local/bin/bindctl

And b10-cmdctl crashed:

Traceback (most recent call last):
  File "/usr/local/libexec/bind10/b10-cmdctl", line 626, in <module>
    run(options.addr, options.port, options.idle_timeout, options.verbose)
  File "/usr/local/libexec/bind10/b10-cmdctl", line 587, in run
    httpd.serve_forever()
  File "/usr/local/lib/python3.2/site-packages/isc/util/socketserver_mixin.py", line 81, in serve_forever
    self._handle_request_noblock();
  File "/usr/local/lib/python3.2/socketserver.py", line 279, in _handle_request_noblock
    request, client_address = self.get_request()
  File "/usr/local/libexec/bind10/b10-cmdctl", line 554, in get_request
    ssl_sock = self._wrap_socket_in_ssl_context(newsocket, key, cert)
  File "/usr/local/libexec/bind10/b10-cmdctl", line 541, in _wrap_socket_in_ssl_context
    ssl_version = ssl.PROTOCOL_SSLv23)
  File "/usr/local/lib/python3.2/ssl.py", line 521, in wrap_socket
    ciphers=ciphers)
  File "/usr/local/lib/python3.2/ssl.py", line 221, in __init__
    self.context.load_cert_chain(certfile, keyfile)
IOError: [Errno 13] Permission denied

Operationally this can/should be fixed by making all of the following
files readable to user "someone":

  • cmdctl-accounts.csv
  • cmdctl-certfile.pem
  • cmdctl-keyfile.pem

but there are a few things to be fixed in the implementation, too:

  • b10-cmdctl shouldn't crash anyway. It should provide more useful/helpful log message about what's wrong and how it should be fixed.
  • the error message from bindctl isn't helpful at all. ideally this should also suggest that it may be a permission problem.

Subtickets

Change History (18)

comment:1 Changed 7 years ago by vorner

Well, I'm not sure if it should or should not crash, since the unreadable
certificate is quite fatal for it. Have it there, sitting and doing nothing is
not useful and crashing may at least bring the admin to the place where to
look. But I agree this should have a better error message with explanation.

As with the bindctl, I don't know. It tries to connect, the connection succeeds
and then, after some initial hello messages the remote just drops the
connection (because it crashes, but it has little other choice than to drop it
when it can't read the cert file). So here it should probably say just that the
other side closed the socket at the wrong moment and suggest looking into the
log if cmdctl is OK.

comment:2 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130122

comment:3 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:4 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

Notes:

  • It is still a fatal error, however, it'll now put a much nicer fatal log message with the exact problem
  • Additionally, the config-time tests are extended (not only existence, but also readability and whether it is a file is checked), so unless files are changed or removed, it shouldn't get to the fatal point in normal usage
  • (note2: it still can't check the *contents* so you will get fatal-upon-connect errors if you configure it to some non-certificate/key file)
  • we can still decide to make it non-fatal, which should now be a much simpler change (but i agree that it should be fatal)
  • apart from adding tests, i fixed a few existing ones around this code, which happened to trigger some expected behaviour, but weren't actually correct imo

comment:5 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 Changed 7 years ago by jinmei

general

  • I think we need a changelog for this ticket.
  • As for whether to handle it as fatal: I'm not necessarily pushing the idea of not doing so, but I simply don't see the point of handling it as fatal. This check is local to that particular request handling. And, as far as I can see there wouldn't be any state left in the cmdctl even if we simply reset the connection and wait for the next request (which will still fail unless the admin fixes the file related issues). On the other hand, if we handle it as fatal, bind10 will restart cmdctl anyway, and cmdctl is in the same state of waiting the next request. So, to me, the "fatal" behavior simply involves kill/restart overhead without any benefit. Furthermore, although it's not immediately clear to me how/whether multiple concurrent clients are handled due to the usual obfuscation of Python server classes, if this means threads are used to accept and handle multiple concurrent clients:
    class SecureHTTPServer(socketserver_mixin.NoPollMixIn,
                           socketserver.ThreadingMixIn,
                           http.server.HTTPServer):
    
    killing the entire process due to a failure of single request doesn't seem to be a good idea in general (although, in practice, if this is about a permission of the cert file, it's quite unlikely that other clients are being handled when the error is detected - but it could still happen). Overall, I can only see disadvantages of treating it as fatal with no clear benefit. Is there any real advantage of handling it as a fatal event?

bindcmd.py

  • Is it okay to not have a test for it?
  • This branch slightly changes failure mode if the following after __try_login fails with socket.error:
                data = response.read().decode()
    
    Previously it's immediately caught and converted to FailToLogin. Now it's propagated to the upper layer and I guess caught here:
            except socket.error as err:
                print('Failed to send request, the connection is closed')
                return 1
    
  • _config_data_check: This comment seems to have to be updated:
                    #TODO, only check whether the file exist,
                    # further check need to be done: eg. whether
                    # the private/certificate is valid.
    
    (it at least does readability check too)

cmdctl_messages.mes

  • missing period?
    While running, b10-cmdctl encountered a fatal exception and it will shut down
    
  • CMDCTL_SSL_SETUP_FAILURE_READING_CERT message: I'd note that depending on whether to treat this case fatal it may have to be updated.

cmdctl_test.py

  • test_check_file: it modifies (the attributes of) a file in the source tree:
            file_name = SRC_FILE_PATH + 'cmdctl-keyfile.pem'
            check_file(file_name)
            with UnreadableFile(file_name):
                self.assertRaises(CmdctlException, check_file, file_name)
    
    Does it work with distcheck?
  • test_wrap_sock_in_ssl_context: not really the subject of this task, but the test doesn't seem to be sufficient: its return value isn't checked; the ssl.SSLError case isn't tested.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

general

  • I think we need a changelog for this ticket.

[bug]
b10-cmdctl no longer aborts on basic file issues with its https certificate or private key file. It performs additional checks, and provides better error if these fail. Additionally, bindctl provides a better error report if it is unable to connect over https connection.

of treating it as fatal with no clear benefit. Is there any real
advantage of handling it as a fatal event?

No, perhaps just that it is slightly more annoying and hence gets looked at sooner, but you're right, unfatalizing now. (by raising socket.error like the other exception clause).

bindcmd.py

  • Is it okay to not have a test for it?

added a separate test for __try_login

  • This branch slightly changes failure mode if the following after __try_login fails with socket.error:
                data = response.read().decode()
    
    Previously it's immediately caught and converted to FailToLogin. Now it's propagated to the upper layer and I guess caught here:
            except socket.error as err:
                print('Failed to send request, the connection is closed')
                return 1
    

oh right, updated (it now performs the read() as well, and returns a tuple on success)

  • _config_data_check: This comment seems to have to be updated:
                    #TODO, only check whether the file exist,
                    # further check need to be done: eg. whether
                    # the private/certificate is valid.
    
    (it at least does readability check too)

right :) updated

cmdctl_messages.mes

  • missing period?
    While running, b10-cmdctl encountered a fatal exception and it will shut down
    

moot now (the message was removed un the unfataling)

  • CMDCTL_SSL_SETUP_FAILURE_READING_CERT message: I'd note that depending on whether to treat this case fatal it may have to be updated.

ack, updated

cmdctl_test.py

  • test_check_file: it modifies (the attributes of) a file in the source tree:
            file_name = SRC_FILE_PATH + 'cmdctl-keyfile.pem'
            check_file(file_name)
            with UnreadableFile(file_name):
                self.assertRaises(CmdctlException, check_file, file_name)
    
    Does it work with distcheck?

as 2631 also noted, it was wrong anyway. So I updated it slightly based on the patch proposed there (but with a BUILD_FILE_PATH instead of two separate values).

  • test_wrap_sock_in_ssl_context: not really the subject of this task, but the test doesn't seem to be sufficient: its return value isn't checked; the ssl.SSLError case isn't tested.

added two additional checks

comment:9 follow-up: Changed 7 years ago by jinmei

changelog

I'd also mention that this could have happened if BIND 10 is installed
with a root privilege and then started with changing the user.

bindcmd.py

  • docstring for __try_login: "to bindctl" should be "to cmdctl"?
            Attempts to log in to bindctl by sending a POST with
    

bindctl_test.py

  • if we directly test __try_login, maybe we should pretend it's "inheritable", i.e., rename it to _try_login with comment. But this is a minor point so I'd leave it to you.
  • maybe we should also check other exception than socket.error is propagated transparently.
  • the warning messages dumped to stdout from tests are a bit noisy. ideally we should introduce a hook (intermediate wrapper method for print or something) so the test can silence or inspect the message. note also that if we inspect the message we can also test if the two cases are handled separately (right now we can't distinguish them because the exception type is the same). but this is also minor, so I'd leave it to you.

cmdctl.py.in

  • I suggest revising the TODO comment as follows (or something):
                    # TODO: we only check whether the file exist, is a
                    # file, and is readable; but further check need to be done:
                    # eg. whether the private/certificate is valid.
    
    to clarify what's we are doing now and what is actually a TODO.
  • shouldn't we close the socket here?
            except (CmdctlException, IOError) as cce:
                logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
                # raise socket error to finish the request
                raise socket.error
    
    like the case of SSLError? In fact, probably due to this bindctl now seems to revert to the previous unhelpful error message.
  • If the answer to the previous question is yes, we might unify the duplicate code as follows:
            except ssl.SSLError as err :
                logger.error(CMDCTL_SSL_SETUP_FAILURE_USER_DENIED, err)
            except (CmdctlException, IOError) as cce:
                logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
            self.close_request(sock)
            # raise socket error to finish the request
            raise socket.error
    
  • In the above code segment, the case for IOError was added. What's the purpose of it? At least this case doesn't seem to be tested.

cmdctl_test.py

  • Should we care about CMDCTL_BUILD_PATH is not in os.environ?
    BUILD_FILE_PATH = '..' + os.sep
    if 'CMDCTL_BUILD_PATH' in os.environ:
        BUILD_FILE_PATH = os.environ['CMDCTL_BUILD_PATH'] + os.sep
    
    I suspect this (whole) test doesn't work reasonably if not run from the Makefile anyway, and when run from Makefile the env variable is always set. Besides, if we really end up using ../, part of the original concern of modifying distributed files still stands.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

changelog

I'd also mention that this could have happened if BIND 10 is installed
with a root privilege and then started with changing the user.

ok

bindcmd.py

  • docstring for __try_login: "to bindctl" should be "to cmdctl"?
            Attempts to log in to bindctl by sending a POST with
    

ack

bindctl_test.py

  • if we directly test __try_login, maybe we should pretend it's "inheritable", i.e., rename it to _try_login with comment. But this is a minor point so I'd leave it to you.

sure

  • maybe we should also check other exception than socket.error is propagated transparently.

ack, randomly chose an ImportError? (since it is quite unlikely that that exception is suddenly raised)

  • the warning messages dumped to stdout from tests are a bit noisy. ideally we should introduce a hook (intermediate wrapper method for print or something) so the test can silence or inspect the message. note also that if we inspect the message we can also test if the two cases are handled separately (right now we can't distinguish them because the exception type is the same). but this is also minor, so I'd leave it to you.

right, something like that was tested in an existing different test already, but by temporarily replacing a standard stream. Replaced it with _print(), and checking output for the _try_login calls

cmdctl.py.in

  • I suggest revising the TODO comment as follows (or something):
                    # TODO: we only check whether the file exist, is a
                    # file, and is readable; but further check need to be done:
                    # eg. whether the private/certificate is valid.
    
    to clarify what's we are doing now and what is actually a TODO.

ack, done

  • shouldn't we close the socket here?
            except (CmdctlException, IOError) as cce:
                logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
                # raise socket error to finish the request
                raise socket.error
    
    like the case of SSLError? In fact, probably due to this bindctl now seems to revert to the previous unhelpful error message.

doh, of course

  • If the answer to the previous question is yes, we might unify the duplicate code as follows:
            except ssl.SSLError as err :
                logger.error(CMDCTL_SSL_SETUP_FAILURE_USER_DENIED, err)
            except (CmdctlException, IOError) as cce:
                logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
            self.close_request(sock)
            # raise socket error to finish the request
            raise socket.error
    

done, added a small comment above the return in the try block that it really does need to return :)

  • In the above code segment, the case for IOError was added. What's the purpose of it? At least this case doesn't seem to be tested.

IOError is what ssl.wrap_socket() raises if it has a problem reading the files. Chances are quite small that this would happen but in theory it is still possible. Added test

cmdctl_test.py

  • Should we care about CMDCTL_BUILD_PATH is not in os.environ?
    BUILD_FILE_PATH = '..' + os.sep
    if 'CMDCTL_BUILD_PATH' in os.environ:
        BUILD_FILE_PATH = os.environ['CMDCTL_BUILD_PATH'] + os.sep
    
    I suspect this (whole) test doesn't work reasonably if not run from the Makefile anyway, and when run from Makefile the env variable is always set. Besides, if we really end up using ../, part of the original concern of modifying distributed files still stands.

I suspect almost none of the python tests do :)

Making it fail if those values aren't set.

comment:12 in reply to: ↑ 11 Changed 7 years ago by jinmei

First, I noticed one (unrelated) style issue and fixed it in the branch.

Replying to jelte:

  • shouldn't we close the socket here?
            except (CmdctlException, IOError) as cce:
                logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
                # raise socket error to finish the request
                raise socket.error
    
    like the case of SSLError? In fact, probably due to this bindctl now seems to revert to the previous unhelpful error message.

doh, of course

Actually, it was still not sufficient. On a closer look, I realized
catching ECONNRESET at bindctl shouldn't help, because in this case
it's not "connection reset" (getting TCP RST). At the TCP level it
would be a close of the connection (FIN), and even that's not really
correct. As the (unhelpful) error message shows, the error was caught
at the SSL level and returned to bindcmd. So what we need to catch is
an sslSSLError exception whose errno is SSL_ERROR_EOF, e.g:

        try:
            response = self.send_POST('/login', param)
            data = response.read().decode()
            return (response, data)
        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self._print("Please check the logs of b10-cmdctl, there may "
                            "be a problem accepting SSL connections, such "
                            "as a permission problem on the server "
                            "certificate file.")
            raise FailToLogin()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            raise FailToLogin()

(note that we'll need some additional tests). I'd also note that in
Python 3.3 this will have to be ssl.SSLEOFError (not sure if the errno
check also works).

Other changes look okay.

comment:13 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Hmz, it appears it depends on the specific python and internal libraries; I get ECONNRESET whether the socket is closed or left open (and closed by scope), and only rarely an ssl.SSLError (it does happen, but about 1 in 100 times... I guess with my system it depends on when exactly it notices).

So I guess the best we could do for now would be to do the 'helpful' message *both* for resets and SSLError.

So I did that. I have tested with python 3.3 and .errno works, however the SSLError.str() has been changed as well, so we can't directly compare strings anymore. Added a small helper function to compare printed messages against a list of regexps.

comment:15 Changed 7 years ago by jinmei

I have two minor comments on the test. With these please merge.

  • __check_printed_messages: variable el is unused so could be _.
  • I guess it's safer to check expected_messages and self.printed_messages has the same length:
        def __check_printed_messages(self, expected_messages):
            '''Helper test function to check the printed messages against a list
               of regexps'''
            self.assertEqual(len(expected_messages), len(self.printed_messages))
    

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:17 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 5.25

comment:18 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks! Did those last two things and merged the branch. closing ticket.

Note: See TracTickets for help on using tickets.