we shouldn't use "assert" to validate incoming data in introducer client #1085

New Issue

zooko · 2010-06-16T03:25:44Z

zooko commented

2010-06-16 03:25:44 +00:00

(http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L163)

Says:


    def _process_announcement(self, ann):
        self._debug_counts["inbound_announcement"] += 1
        (furl, service_name, ri_name, nickname_utf8, ver, oldest) = ann
        if service_name not in self._subscribed_service_names:
            self.log("announcement for a service we don't care about [%s]"
                     % (service_name,), level=log.UNUSUAL, umid="dIpGNA")
            self._debug_counts["wrong_service"] += 1
            return
        self.log("announcement for [%s]: %s" % (service_name, ann),
                 umid="BoKEag")
        assert type(furl) is str
        assert type(service_name) is str
        assert type(ri_name) is str
        assert type(nickname_utf8) is str
        nickname = nickname_utf8.decode("utf-8")
        assert type(nickname) is unicode
        assert type(ver) is str
        assert type(oldest) is str

This means that validation of this incoming data is turned off by the PYTHONOPTIMIZE setting, and it means that introducers have the power to cause AssertionFailure to be raised from this function. Now, in practice causing AssertionFailure to be raised from this function won't hurt anything in the current version, but this is still not the right way to do it. We would like for failures of any of these validations to result in an exception that will get logged as explicitly being "we received an ill-formed announcement" rather than "AssertionError", which is supposed to mean "there was an internal error in our source code".

(Note that the introduction server may well just be passing this announcement through as it was given to the introduction server by someone else, and may not be responsible for the ill-formedness itself...)

Relatedly, this code shouldn't catch all possible kinds of exceptions:

http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L149

            try:
                self._process_announcement(ann)
            except:
                log.err(format="unable to process announcement %(ann)s",
                        ann=ann)

But should instead catch the specific kind of exception that means "we received an ill-formed announcement". Any other kind of exception, including AssertionFailure, should not be caught here.

(http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L163) Says: ``` def _process_announcement(self, ann): self._debug_counts["inbound_announcement"] += 1 (furl, service_name, ri_name, nickname_utf8, ver, oldest) = ann if service_name not in self._subscribed_service_names: self.log("announcement for a service we don't care about [%s]" % (service_name,), level=log.UNUSUAL, umid="dIpGNA") self._debug_counts["wrong_service"] += 1 return self.log("announcement for [%s]: %s" % (service_name, ann), umid="BoKEag") assert type(furl) is str assert type(service_name) is str assert type(ri_name) is str assert type(nickname_utf8) is str nickname = nickname_utf8.decode("utf-8") assert type(nickname) is unicode assert type(ver) is str assert type(oldest) is str ``` This means that validation of this incoming data is turned off by the PYTHONOPTIMIZE setting, and it means that introducers have the power to cause AssertionFailure to be raised from this function. Now, in practice causing AssertionFailure to be raised from this function won't hurt anything in the current version, but this is still not the right way to do it. We would like for failures of any of these validations to result in an exception that will get logged as explicitly being "we received an ill-formed announcement" rather than "AssertionError", which is supposed to mean "there was an internal error in our source code". (Note that the introduction server may well just be passing this announcement through as it was given to the introduction server by someone else, and may not be responsible for the ill-formedness itself...) Relatedly, this code shouldn't catch all possible kinds of exceptions: <http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L149> ``` try: self._process_announcement(ann) except: log.err(format="unable to process announcement %(ann)s", ann=ann) ``` But should instead catch the specific kind of exception that means "we received an ill-formed announcement". Any other kind of exception, including AssertionFailure, should not be caught here.

zooko added the

labels 2010-06-16 03:25:44 +00:00

zooko added this to the soon milestone 2010-06-16 03:25:44 +00:00

warner commented

2010-06-16 18:02:46 +00:00

I agree, although I'll mention that most of the time that I add asserts, it's just to move exceptions closer to the point of the bug. So I wasn't specifically trying to guard against malicious behavior on the part of the Introducer or some other publisher, rather I just wanted the exception (especially during testing) to be less confusing.

Making real exception-catching code frequently makes me think I need to invent a new exception type for that case, then catch it and do something useful with it elsewhere. I should probably be less worried about that: in this case, raising TypeError or ValueError would be sufficient, and even letting the exception bubble all the way up to the foolscap callRemote handler would be good enough. That would get logged.

Incidentally, whoever touches this code next: that client.py try/except on line 149 should be replaced with an 'except BaseException', or whatever it is that doesn't catch SIGINT and out-of-memory exceptions. Also, the log.err needs to use %(ann)r instead of %(ann)s (and we need to test that that actually works in foolscap.logging and 'flogtool dump'), or some other checking should be done, to make sure that the logging call can tolerate a non-stringable-in-some-output-encoding announcement string (imagine weird binary stuff in an announcement).

I agree, although I'll mention that most of the time that I add asserts, it's just to move exceptions closer to the point of the bug. So I wasn't specifically trying to guard against malicious behavior on the part of the Introducer or some other publisher, rather I just wanted the exception (especially during testing) to be less confusing. Making real exception-catching code frequently makes me think I need to invent a new exception type for that case, then catch it and do something useful with it elsewhere. I should probably be less worried about that: in this case, raising TypeError or ValueError would be sufficient, and even letting the exception bubble all the way up to the foolscap callRemote handler would be good enough. That would get logged. Incidentally, whoever touches this code next: that client.py try/except on line 149 should be replaced with an 'except [BaseException](wiki/BaseException)', or whatever it is that doesn't catch SIGINT and out-of-memory exceptions. Also, the log.err needs to use %(ann)r instead of %(ann)s (and we need to test that that actually works in foolscap.logging and 'flogtool dump'), or some other checking should be done, to make sure that the logging call can tolerate a non-stringable-in-some-output-encoding announcement string (imagine weird binary stuff in an announcement).

exarkun commented

2020-01-17 14:30:29 +00:00

Essentially a duplicate of ticket:1944 and ticket:1968

exarkun added the

duplicate

label 2020-01-17 14:30:29 +00:00

exarkun closed this issue

2020-01-17 14:30:29 +00:00

Sign in to join this conversation.