Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pyscard mutes/hides user created exceptions in on_insert() hook. #167

Closed
Torxed opened this issue Apr 11, 2024 · 5 comments
Closed

pyscard mutes/hides user created exceptions in on_insert() hook. #167

Torxed opened this issue Apr 11, 2024 · 5 comments

Comments

@Torxed
Copy link

Torxed commented Apr 11, 2024

Your system information

  • Operating system used: Arch Linux
  • pyscard version: 2.0.9
  • Python version: 3.11.8

Please describe your issue in as much detail as possible:

The following exception handling mutes issues originating outside of pyscard.
Given the traceback below, the following would have been muted:

Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/smartcard/CardMonitoring.py", line 180, in run
    self.observable.notifyObservers(
  File "/usr/lib/python3.11/site-packages/smartcard/Observer.py", line 59, in notifyObservers
    observer.update(self, arg)
  File "test.py", line 24, in update
    self.on_insert(addedcards)
  File "test.py", line 117, in on_insert
    self.on_correct_identity(self)
  File "test.py", line 150, in _load_shares
    print(session.use_in_some_way())
          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "test_lib.py", line 19, in shares
    print(session.sign(payload, privkey))
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: UserSession.sign() takes 2 positional arguments but 3 were given

The correct thing here would be to catch it in test_lib.py obviously.
But the error will silently go by until a explicit try/catch is put into place in test_lib.py, making it quite difficult to know where to even put the exception handling. Debugging these errors become quite tedious even with proper breakpoints unless the exception becomes visible to the developing user.

Either pyscard shouldn't do blind exception handling with pass as the result, or the exception of dealing with __del__() should be dealt with conditional checks rather than relying on handling the exception.

Steps for reproducing this issue:

I know this isn't ideal, but I'm in no able to create a minimal working example at the moment due to time. I can come back later and add one, but the exception itself should be clear enough to understand that the exception handling situation isn't ideal.

Apologies for this, but time is extremely limited at the moment.

@LudovicRousseau
Copy link
Owner

How did you get the traceback then?
You used a modified pyscard?

Can you share your test.py source code?

@Torxed
Copy link
Author

Torxed commented Apr 18, 2024

Will do in a day or so.
But yes, modified the above linked code to look like this:

  except TypeError:
-     pass
+     traceback.print_exc()
  except AttributeError:
-     pass
+     traceback.print_exc()

A random shot in the dark is, that it's happening when you're running "external dependencies" in threads but they relate to the on_insert call in pyscard. I know this is a fuzzy description and I'll describe it better later - or even better with some code in a day or so.

@Torxed
Copy link
Author

Torxed commented May 5, 2024

Here's an example of where the above "muted error" occurs.
This is as small as I could make the example with the time I had.

To run it:

  1. Ensure pcscd is running: /usr/bin/pcscd --foreground --auto-exit $PCSCD_ARGS
  2. Insert your PKCS#11 compliant smartcard (since pyscard supports treating it as "injected" on first execution)
  3. Make sure pkcs11-tool can find your smartcard: pkcs11-tool -L
  4. python <code below>.py

Without the patched code, you'll get:

screenshot1

And after patching it, you'll hopefully get:

screenshot2

(in the screenshot above I just added print() instead of traceback.print_exc() because I was in a rush)
And here's the code for testing/verifying, and I used a YubiKey to load the privkey, cert and everything else using the PIV module:

import time
import threading
import cryptography.x509
import cryptography.hazmat.primitives
import smartcard
import smartcard.CardMonitoring
from PyKCS11 import *
from PyKCS11.LowLevel import CKA_ID

yubikey_pin = "123456"
pkcs11_lib = '/usr/lib/opensc-pkcs11.so'
test = {
	"alive": True
}

class CardInsertedHook(smartcard.CardMonitoring.CardObserver):
	def __init__(self, on_insert=None, on_eject=None):
		self.on_insert = on_insert
		self.on_eject = on_eject

	def update(self, observable, actions):
		(addedcards, removedcards) = actions
		if addedcards and self.on_insert:
			self.on_insert(addedcards)
		if removedcards and self.on_eject:
			self.on_eject(removedcards)

class CardMonitor(threading.Thread):
	def __init__(self, on_insert=None, on_eject=None):
		# Create an instance of the CardMonitor
		cardmonitor = smartcard.CardMonitoring.CardMonitor()

		# Add your observer to the CardMonitor
		cardobserver = CardInsertedHook(on_insert, on_eject)
		cardmonitor.addObserver(cardobserver)

		threading.Thread.__init__(self)
		self.start()

	def run(self):
		# We stay alive as long as our overlord says to stay alive
		while test['alive']:
			time.sleep(0.25)

class YubikeySession:
	def __init__(self):
		self.session = None
		self.share_holders = {}

		# Initialize the library
		self.pkcs11 = PyKCS11Lib()
		self.pkcs11.load(pkcs11_lib)

	def sign(self, data :bytes):
		print(f"Signing {data} with {self.session}")

	def decrypt(self, data :bytes):
		print(f"Decrypt {data} with {self.session}")

	def on_insert(self, cards):
		for device in self.pkcs11.getSlotList():
			try:
				_tmp_session = self.pkcs11.openSession(device, CKF_SERIAL_SESSION | CKF_RW_SESSION)
			except PyKCS11.PyKCS11Error as error:
				if error.value == 224:
					continue
				raise error

			try:
				_tmp_session.login(yubikey_pin)
			except PyKCS11.PyKCS11Error as error:
				if error.value == 256:
					# Already logged in
					pass
				else:
					raise error

			certificates = _tmp_session.findObjects([
				(CKA_CLASS, CKO_CERTIFICATE),
			])

			for certificate in certificates:
				_tmp_cert_der = bytes(_tmp_session.getAttributeValue(certificate, [CKA_VALUE])[0])

			_tmp_private_keys = _tmp_session.findObjects([
				(CKA_CLASS, CKO_PRIVATE_KEY),
			])

			for _tmp_private_key in _tmp_private_keys:
				attributes = _tmp_session.getAttributeValue(_tmp_private_key, [CKA_ID])

				_tmp_cert = cryptography.x509.load_der_x509_certificate(_tmp_cert_der)
				_tmp_fingerprint = _tmp_cert.fingerprint(cryptography.hazmat.primitives.hashes.SHA512())
				break # .. todo:: make sure this belongs to the cert we found

			print(f"Has detected Yubikey for {_tmp_cert.subject.get_attributes_for_oid(cryptography.x509.NameOID.COMMON_NAME)[0].value}")

			self.session = _tmp_session
			break

		if self.session:
			self.sign(b"data", "This should break things")

	def on_eject(self, cards):
		if self.session:
			self.session.logout()
			self.session.closeSession()
			self.session = None
			
yubikey = YubikeySession()
sc_monitor = CardMonitor(on_insert=yubikey.on_insert, on_eject=yubikey.on_eject)

time.sleep(2)

yubikey.on_eject([])
test['alive'] = False

LudovicRousseau added a commit to LudovicRousseau/pyscard-devel that referenced this issue May 8, 2024
The exceptions were supposed to be gnerated by the __del__() method.
But the __del__() method was already commented out since
9f2198c in 2007.

Thanks to Anton Hvornum for the bug report
" pyscard mutes/hides user created exceptions in on_insert() hook. #167 "
Closes: LudovicRousseau/pyscard#167
@LudovicRousseau
Copy link
Owner

Fixed.
Thanks

If you are waiting for a PKCS#11 slot insertion or removal I would suggest to use waitForSlotEvent() https://pkcs11wrap.sourceforge.io/api/api.html#PyKCS11.PyKCS11Lib.waitForSlotEvent from PyKCS11.
No need to use PySCard for that.

See Get token events sample code: https://github.com/LudovicRousseau/PyKCS11/blob/master/samples/events.py

@Torxed
Copy link
Author

Torxed commented May 9, 2024

No worries, and great work but also thanks for being patient!

And waitForSlotEvent() is spot on what I need by the looks of it, will for sure give that a go. Thanks for nudging me in the right direction, looks a lot cleaner than what I'm doing currently.

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

No branches or pull requests

2 participants