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

Add bitcoin address label to request payment QR code #7636

Merged
merged 1 commit into from Jun 14, 2016

Conversation

Projects
None yet
7 participants
@makevoid
Contributor

makevoid commented Mar 3, 2016

This is a change for the QT wallet

Why this change?
This way the user can be sure that the QR code scanner app he/she's using is not displaying a different address to maliciously steal the funds.

I've seen an android app in the wild that is a simple qr code scanner but when it scans a bitcoin address it replaces it with a different address belonging to the hacker. I'm starting to get worried of bitcoin address qr codes without a label that states the address in clear so that the user can always check that he/she is depositing the funds to the desired address.
That's why I decided to modify the bitcoin qt code.


Code changes:

  • renamed myImage to qrImage
  • added qrAddrImage in which the bitcoin address label is painted onto
  • scaled the saved pixmap with antialiasing to improve readability of the text and the scanning of the qrcode
@makevoid

This comment has been minimized.

Show comment
Hide comment
@makevoid

makevoid Mar 3, 2016

Contributor

That's the preview of the change, the qr image has the bitcoin address label underneath so that when it's saved you can match it:

Contributor

makevoid commented Mar 3, 2016

That's the preview of the change, the qr image has the bitcoin address label underneath so that when it's saved you can match it:

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2016

Member

Haven't looked at the code, but screenshot looks good to me, nice work.

Member

laanwj commented Mar 3, 2016

Haven't looked at the code, but screenshot looks good to me, nice work.

@laanwj laanwj added the GUI label Mar 3, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2016

Member

Thanks!

Having a malicious QRCode can probably only happen if you system, Bitcoin-Core or at least libqrencode is compromise.

Your PR would only protect from an attack on libqrencode and only if the user uses a self-compiled Bitcoin-Core (official binaries are static linked). Because, if you could tamper Bitcoin-Core, you could change the address at a core point which would also provide a malicious address below the QRCode.

This change would def. make sense if we would have HD for key derivation.
You could use a smartphone with the same xpub master key and verify the address at given keypath by comparing QRCodes or addresses together with the keypath.

IMO this PR does not hurt, although I don't believe that it increases security.

UtACK.

Member

jonasschnelli commented Mar 3, 2016

Thanks!

Having a malicious QRCode can probably only happen if you system, Bitcoin-Core or at least libqrencode is compromise.

Your PR would only protect from an attack on libqrencode and only if the user uses a self-compiled Bitcoin-Core (official binaries are static linked). Because, if you could tamper Bitcoin-Core, you could change the address at a core point which would also provide a malicious address below the QRCode.

This change would def. make sense if we would have HD for key derivation.
You could use a smartphone with the same xpub master key and verify the address at given keypath by comparing QRCodes or addresses together with the keypath.

IMO this PR does not hurt, although I don't believe that it increases security.

UtACK.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2016

Member

IMO this PR does not hurt, although I don't believe that is increases security.

If anything at the printer side is compromised, you're out of luck. But what about a malicious QR code reader? I think it could help there, comparing the address before accepting the transaction in your wallet.

Member

laanwj commented Mar 3, 2016

IMO this PR does not hurt, although I don't believe that is increases security.

If anything at the printer side is compromised, you're out of luck. But what about a malicious QR code reader? I think it could help there, comparing the address before accepting the transaction in your wallet.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 3, 2016

Member

Right, I think it makes sense to have a human readable version of the QR code besides it. Concept ACK.

Member

MarcoFalke commented Mar 3, 2016

Right, I think it makes sense to have a human readable version of the QR code besides it. Concept ACK.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2016

Member

Yes. I just thought people could already do this because they have the address below the QRCode already. But this PR makes it more prominent available.

Member

jonasschnelli commented Mar 3, 2016

Yes. I just thought people could already do this because they have the address below the QRCode already. But this PR makes it more prominent available.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 3, 2016

Contributor

Testing on OS X. The image is blurred. Original image is OK:

Current master:
before

This PR:
after

The Request payment window shows only the top part of the address:

screen shot 2016-03-03 at 13 43 03

Can you fix these problems?

Contributor

paveljanik commented Mar 3, 2016

Testing on OS X. The image is blurred. Original image is OK:

Current master:
before

This PR:
after

The Request payment window shows only the top part of the address:

screen shot 2016-03-03 at 13 43 03

Can you fix these problems?

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/receiverequestdialog.cpp
@makevoid

This comment has been minimized.

Show comment
Hide comment
@makevoid

makevoid Mar 3, 2016

Contributor

I removed the anti-aliasing (I should I've imagined that it was bad for qr code scanning) and now it shouldn't have that blurring problem on OSX as it uses the same size as the size it's using to display it (300), anyway I will double-check it in few hours on that OS.

The only thing that's changed now is the export size, the final image is 44 pixel larger for each dimension. The good thing that is now I can reuse the constant EXPORT_IMAGE_SIZE (now changed to QR_IMAGE_SIZE, used only in this file) for the dimensions, also there's no need to re-scale the image again and again anymore.

I can squash the three commits to a single one and force push to this branch when needed.

Contributor

makevoid commented Mar 3, 2016

I removed the anti-aliasing (I should I've imagined that it was bad for qr code scanning) and now it shouldn't have that blurring problem on OSX as it uses the same size as the size it's using to display it (300), anyway I will double-check it in few hours on that OS.

The only thing that's changed now is the export size, the final image is 44 pixel larger for each dimension. The good thing that is now I can reuse the constant EXPORT_IMAGE_SIZE (now changed to QR_IMAGE_SIZE, used only in this file) for the dimensions, also there's no need to re-scale the image again and again anymore.

I can squash the three commits to a single one and force push to this branch when needed.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 3, 2016

Contributor

One new warning generated:

+qt/receiverequestdialog.cpp:184:73: warning: implicit conversion from 'double' to 'int' changes value from 8.5 to 8 [-Wliteral-conversion]
+            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8.5));
+                            ~~~~~                                       ^~~
+1 warning generated.
Contributor

paveljanik commented Mar 3, 2016

One new warning generated:

+qt/receiverequestdialog.cpp:184:73: warning: implicit conversion from 'double' to 'int' changes value from 8.5 to 8 [-Wliteral-conversion]
+            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8.5));
+                            ~~~~~                                       ^~~
+1 warning generated.
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 3, 2016

Member

So now the window displays the address twice? Maybe just add it to the image when it's being saved...?

Member

luke-jr commented Mar 3, 2016

So now the window displays the address twice? Maybe just add it to the image when it's being saved...?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 3, 2016

Contributor

@luke-jr If displayed properly, the address is displayed four times! Title bar, image, bitcoin: URL and address. 8)

Contributor

paveljanik commented Mar 3, 2016

@luke-jr If displayed properly, the address is displayed four times! Title bar, image, bitcoin: URL and address. 8)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2016

Member

Hmm... font feels a bit small on my end (HiDPI/retina OSX):

bildschirmfoto 2016-03-03 um 17 06 04

Member

jonasschnelli commented Mar 3, 2016

Hmm... font feels a bit small on my end (HiDPI/retina OSX):

bildschirmfoto 2016-03-03 um 17 06 04

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/receiverequestdialog.cpp
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2016

Member

So now the window displays the address twice? Maybe just add it to the image when it's being saved...?

Well at least people can't complain they're missing the address :-)
I don't think this is much of a concern, it's not like the window is short on space.

Member

laanwj commented Mar 3, 2016

So now the window displays the address twice? Maybe just add it to the image when it's being saved...?

Well at least people can't complain they're missing the address :-)
I don't think this is much of a concern, it's not like the window is short on space.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2016

Member

One thing I do wonder about though: do QR scanners need an empty (white) border around a QR code? If so, how large should it be?

Member

laanwj commented Mar 3, 2016

One thing I do wonder about though: do QR scanners need an empty (white) border around a QR code? If so, how large should it be?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2016

Member

@laanwj: I recently played around with max QR size/bytes, black/white (inverted) and the white-space-border. I could not find any specification about the "quite zone" (white border), but I guess "one square width" as min border size should be more as sufficient. I guess it depends on the QRCode reader implementation. But I could not read the QRCode with common readers if there where no white borders.

Member

jonasschnelli commented Mar 3, 2016

@laanwj: I recently played around with max QR size/bytes, black/white (inverted) and the white-space-border. I could not find any specification about the "quite zone" (white border), but I guess "one square width" as min border size should be more as sufficient. I guess it depends on the QRCode reader implementation. But I could not read the QRCode with common readers if there where no white borders.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 3, 2016

Member
Member

sipa commented Mar 3, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 3, 2016

Member
Member

sipa commented Mar 3, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 10, 2016

Contributor

Current status:

screen shot 2016-03-10 at 13 50 18

Contributor

paveljanik commented Mar 10, 2016

Current status:

screen shot 2016-03-10 at 13 50 18

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 13, 2016

Member

Still not really readable on retina/HiDPI OSX:
bildschirmfoto 2016-03-13 um 10 24 24

Member

jonasschnelli commented Mar 13, 2016

Still not really readable on retina/HiDPI OSX:
bildschirmfoto 2016-03-13 um 10 24 24

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/receiverequestdialog.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 5, 2016

Member

ping @makevoid: I think this PR is almost ready. We just need a way that the font size respects the screen density (HiDPI).

Member

jonasschnelli commented Apr 5, 2016

ping @makevoid: I think this PR is almost ready. We just need a way that the font size respects the screen density (HiDPI).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Jun 2, 2016

ping @makevoid

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2016

Member

We just need a way that the font size respects the screen density (HiDPI).

I'd say no because this is exported to an image file. We don't want the output to be dependent on the user's screen resolution.

Member

laanwj commented Jun 13, 2016

We just need a way that the font size respects the screen density (HiDPI).

I'd say no because this is exported to an image file. We don't want the output to be dependent on the user's screen resolution.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 13, 2016

Member

@laanwj: check #7636 (comment)
The font size on HiDPI is different then on non-HiDPI.

I can take care about this and finish this PR.

Member

jonasschnelli commented Jun 13, 2016

@laanwj: check #7636 (comment)
The font size on HiDPI is different then on non-HiDPI.

I can take care about this and finish this PR.

@makevoid

This comment has been minimized.

Show comment
Hide comment
@makevoid

makevoid Jun 13, 2016

Contributor

Sorry but I don't have an HiDPI screen to test with yet, is there anything I can do? should I squash the commits?

Contributor

makevoid commented Jun 13, 2016

Sorry but I don't have an HiDPI screen to test with yet, is there anything I can do? should I squash the commits?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2016

Member

The font size on HiDPI is different then on non-HiDPI.

Just a matter of misunderstanding / wrong formulation then - what we want here is to use a font size that is fixed in pixel size, just like the QR code itself is.

Member

laanwj commented Jun 13, 2016

The font size on HiDPI is different then on non-HiDPI.

Just a matter of misunderstanding / wrong formulation then - what we want here is to use a font size that is fixed in pixel size, just like the QR code itself is.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 13, 2016

Member

The QRCode is not rendered HiDPI (not required). Setting the font size in pixel works (because the size of the QImage is fixed in pixels not in points).
The only little downside is the missing HiDPI support for the new address (looks a bit pixled here).

bildschirmfoto 2016-06-13 um 15 41 25

bildschirmfoto 2016-06-13 um 15 40 57

Here's the diff.
@makevoid: Can you apply and squash?

diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
index 1e104bb..b81a039 100644
--- a/src/qt/receiverequestdialog.cpp
+++ b/src/qt/receiverequestdialog.cpp
@@ -183,7 +183,9 @@ void ReceiveRequestDialog::update()
             qrAddrImage.fill(0xffffff);
             QPainter painter(&qrAddrImage);
             painter.drawImage(0, 0, qrImage.scaled(QR_IMAGE_SIZE, QR_IMAGE_SIZE));
-            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8));
+            QFont font = QFont(GUIUtil::fixedPitchFont().toString());
+            font.setPixelSize(12);
+            painter.setFont(font);
             QRect paddedRect = qrAddrImage.rect();
             paddedRect.setHeight(QR_IMAGE_SIZE+12);
             painter.drawText(paddedRect, Qt::AlignBottom|Qt::AlignCenter, info.address);
Member

jonasschnelli commented Jun 13, 2016

The QRCode is not rendered HiDPI (not required). Setting the font size in pixel works (because the size of the QImage is fixed in pixels not in points).
The only little downside is the missing HiDPI support for the new address (looks a bit pixled here).

bildschirmfoto 2016-06-13 um 15 41 25

bildschirmfoto 2016-06-13 um 15 40 57

Here's the diff.
@makevoid: Can you apply and squash?

diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
index 1e104bb..b81a039 100644
--- a/src/qt/receiverequestdialog.cpp
+++ b/src/qt/receiverequestdialog.cpp
@@ -183,7 +183,9 @@ void ReceiveRequestDialog::update()
             qrAddrImage.fill(0xffffff);
             QPainter painter(&qrAddrImage);
             painter.drawImage(0, 0, qrImage.scaled(QR_IMAGE_SIZE, QR_IMAGE_SIZE));
-            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8));
+            QFont font = QFont(GUIUtil::fixedPitchFont().toString());
+            font.setPixelSize(12);
+            painter.setFont(font);
             QRect paddedRect = qrAddrImage.rect();
             paddedRect.setHeight(QR_IMAGE_SIZE+12);
             painter.drawText(paddedRect, Qt::AlignBottom|Qt::AlignCenter, info.address);
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 13, 2016

Member

+ QFont font = QFont(GUIUtil::fixedPitchFont().toString());

fixedPitchFont() already returns QFont. Is this odd constructor necessary?

Member

MarcoFalke commented Jun 13, 2016

+ QFont font = QFont(GUIUtil::fixedPitchFont().toString());

fixedPitchFont() already returns QFont. Is this odd constructor necessary?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 13, 2016

Member

@MarcoFalke: Oh yes.
@makevoid: please use QFont font = GUIUtil::fixedPitchFont();.

Member

jonasschnelli commented Jun 13, 2016

@MarcoFalke: Oh yes.
@makevoid: please use QFont font = GUIUtil::fixedPitchFont();.

Add address label to request payment QR Code (QT)
In the Receive 'Tab' of the QT wallet, when 'Show'ing a previously requested payment, add a label underneath the QR Code showing the bitcoin address where the funds will go to.

This way the user can be sure that the QR code scanner app the user using is reading the correct bitcoin address, preventing funds to be stolen.

Includes fix for HiDPI screens by @jonasschnelli.
@makevoid

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Jun 13, 2016

utACK 1c2a1ba

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 14, 2016

Member

Tested ACK 1c2a1ba.

Looks also good on Win(10).
HiDPI issues on windows are unrelated to this PR (we need Qt5.6.1).

bildschirmfoto 2016-06-14 um 13 03 32

Member

jonasschnelli commented Jun 14, 2016

Tested ACK 1c2a1ba.

Looks also good on Win(10).
HiDPI issues on windows are unrelated to this PR (we need Qt5.6.1).

bildschirmfoto 2016-06-14 um 13 03 32

@jonasschnelli jonasschnelli merged commit 1c2a1ba into bitcoin:master Jun 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jun 14, 2016

Merge #7636: Add bitcoin address label to request payment QR code
1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge bitcoin#7636: Add bitcoin address label to request payment QR code
1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#7636: Add bitcoin address label to request payment QR code
1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

codablock added a commit to codablock/dash that referenced this pull request Dec 27, 2017

Merge bitcoin#7636: Add bitcoin address label to request payment QR code
1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

codablock added a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge bitcoin#7636: Add bitcoin address label to request payment QR code
1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment