Opened 8 months ago

Closed 2 months ago

#31967 closed defect (fixed)

BridgeDB Server uses insecure pseudorandom generator for selecting cached captcha

Reported by: willbarr Owned by:
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version: sbws: unspecified
Severity: Normal Keywords:
Cc: phw, cohosh, agix Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

https://gitweb.torproject.org/bridgedb.git/tree/bridgedb/captcha.py#n389

From python documentation: The pseudo-random generators of this module (random) should not be used for security purposes.

It should use the secrets module secrets.choice() or if you plan to keep python2 compatibility random.SystemRandom.choice().

Child Tickets

Attachments (3)

Change History (15)

comment:1 Changed 3 months ago by agix

Subject: [PATCH] Fix for #31967. Changed pseudo-random generator to

    random.SystemRandom.choice()

---

    bridgedb/captcha.py | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bridgedb/captcha.py b/bridgedb/captcha.py
index b66972c..485974b 100644
--- a/bridgedb/captcha.py
+++ b/bridgedb/captcha.py
@@ -386,7 +386,7 @@ class GimpCaptcha(Captcha):

    and a challenge string (used for checking the client's solution).

    """
    try:

    imageFilename = random.choice(os.listdir(self.cacheDir)) 

+ imageFilename = random.SystemRandom().choice(os.listdir(self.cacheDir))

    imagePath = os.path.join(self.cacheDir, imageFilename)
    with open(imagePath) as imageFile:

        self.image = imageFile.read()

-- 
2.17.1
Last edited 3 months ago by dcf (previous) (diff)

comment:2 Changed 3 months ago by cohosh

Status: newneeds_review

comment:3 Changed 3 months ago by cohosh

Cc: cohosh agix added

comment:4 Changed 3 months ago by dcf

Status: needs_reviewneeds_revision

It looks like the patch in comment:1 has some whitespace/indentation problems. It might just be mangled by copy-and-paste. Try making a commit in your local repo, and then run git format-patch HEAD^!. This will create a patch file that you can attach and avoid any problems with pasting into a text field.

It looks like you need to revise the patch to remove the random.choice line, since you added the random.SystemRandom().choice replacement.

comment:5 Changed 3 months ago by agix

I attached the patch as you said, thanks for the help dcf.

comment:6 Changed 2 months ago by phw

Status: needs_revisionneeds_information

Thanks for your work, agix!

Hmm, the patch doesn't apply onto the develop branch (currently at ef9ea5c). What commit did you use as base for this patch?

Changed 2 months ago by agix

comment:7 Changed 2 months ago by agix

Sorry for that!

I forgot to pull the latest changes, that's why the patch wont apply.
This one should work though.

comment:8 Changed 2 months ago by agix

Status: needs_informationneeds_review

comment:9 Changed 2 months ago by phw

Status: needs_reviewneeds_revision

The patch removes the line that initialises imagePath. That's not supposed to happen, right? Also, there's a tab at the beginning of the newly added line, followed by spaces, which breaks indentation. Can you please fix these two issues?

The patch subject should probably not be "Fixed broken patch" because it's not fixing a broken patch. Rather, it's changing the RNG :)

comment:10 Changed 2 months ago by agix

Well I guess this makes me officially the poster child for human stupidity.
Nevertheless, here is finally the correct patch.
Sorry again for this mess.

comment:11 Changed 2 months ago by agix

Status: needs_revisionneeds_review

comment:12 in reply to:  10 Changed 2 months ago by phw

Resolution: fixed
Status: needs_reviewclosed

Replying to agix:

Well I guess this makes me officially the poster child for human stupidity.


Don't worry, this happens to the best of us :)

Nevertheless, here is finally the correct patch.


Thanks, the patch looks good to me! I merged it into develop and added a CHANGELOG entry. I'll deploy the patch in the next few days, once I had time to fix the broken installation process.

Note: See TracTickets for help on using tickets.