Skip to content

tabs to spaces, and add some input validation#2

Open
fbaew wants to merge 1 commit intoProtospace:masterfrom
fbaew:sanitize-input
Open

tabs to spaces, and add some input validation#2
fbaew wants to merge 1 commit intoProtospace:masterfrom
fbaew:sanitize-input

Conversation

@fbaew
Copy link

@fbaew fbaew commented Feb 28, 2018

First of all, I'm not trying to start a holy war but I noticed too late that my IDE converted all the tabs in __init__.py to spaces and I couldn't be bothered making that a separate commit. If you care about this fix I will happily:

  1. Revert to the old funky mix of tabs and spaces and apply my changes against that --or--
  2. Convert the rest of the repo to space indentation

I'm also happy to help with any testing/deployment effort.

Since it's hard to tell from the diff (due to all the tab->space business) the actual fix is in lines 77-103 of server/__init__.py:

    def is_sane(self, card):
        """
		do a modicum of validation to avoid SQL injection or other abuse.
		allowed_characters may need to be tweaked if card IDs may include
		other characters...

		:returns: A boolean indicating whether the card input is
				  indeed a sane value.
		"""

        allowed_characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'

        if len(card) != 10:
            return False

        for char in card.upper():
            if char not in allowed_characters:
                return False

    def loop(self):
        card = self.serial.readline()
        if not card:
            return

        card = card.strip()
        if not self.is_sane(card):
            return

        ...

Not sure if it's really of any practical concern, but the current implementation is vulnerable to SQL injection (either by way of a specially-crafted access card or by ripping off the card reader and interfacing with the pi directly). For example a card ID like 'or''=''-- satisfies the "card ID must be 10 characters" requirement, but would result in a random (or maybe the first) card record in the DB being retrieved, possibly allowing access to the building. I'm not too strong on the hardware side but I think at least in the second scenario it is actually possible to inject arbitrary bytes (maybe the RFID reader always outputs hex characters but an attacker could put whatever they wanted on the wire).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant