Bug#902935: units_cur: missing input validation
* Adrian Mariano , 2018-07-23, 20:22:
I had sort of figured that getting bogus price data was a more serious
error than having extra or missing currencies, so I had made that error
message unconditional.
Good call.
stderr.write('Got unknown metal "{}" with value "{}"\n',metal,price)
This would fail with:
TypeError: function takes exactly 1 argument (3 given)
I think you wanted to use the format() method here. :-)
Other than that, it looks good to me.
--
Jakub Wilk
Bug#902935: units_cur: missing input validation
On Mon, Jul 23, 2018 at 05:11:04PM +0200, Jakub Wilk wrote:
> * Adrian Mariano , 2018-07-22, 18:04:
> > > > I'm not sure about exactly the right way to validate the metals.
> > > > I took the most relaxed route of just banning '!',
> > > Enumerating badness makes me nervous. It is generally considered a
> > > bad security practice.
> >
> > What do you mean by "enumerating badness"?
>
> I mean forbidding only things that are known to be unsafe (as opposed to
> only allowing things that are known to be safe). The term was popularized by
> this essay:
> https://www.ranum.com/security/computer_security/editorials/dumb/
>
> > for metal, price in metals.items():
> > if metal in validmetals:
> >metallist.append('{:19}{} US$/troyounce'.format( metal + 'price', price))
>
> Price is not validated here.
>
> >stderr.write('Got unknown metal "{}" with value "{}"\n',metal,price)
>
> I think this message should be printed only if --verbose was enabled, for
> consistency how unknown currencies are handled.
Ok. Trying yet again. New version attached. I had sort of figured
that getting bogus price data was a more serious error than having
extra or missing currencies, so I had made that error message
unconditional. Do you think it's the wrong choice? (The errors about
failed network connections are also unconditional.)
#!/usr/bin/python
#
# units_cur for units, a program for updated currency exchange rates
#
# Copyright (C) 2017-2018
# Free Software Foundation, Inc
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
#
#
# This program was written by Adrian Mariano (adri...@gnu.org)
#
# For Python 2 & 3 compatibility
from __future__ import absolute_import, division, print_function
#
#
version = '4.3'
# Version 4.3: 20 July 2018
#
# Validate rate data from server
#
# Version 4.2: 18 April 2018
#
# Handle case of empty/malformed entry returned from the server
#
# Version 4.1: 30 October 2017
#
# Fixed to include USD in the list of currency codes.
#
# Version 4: 2 October 2017
#
# Complete rewrite to use Yahoo YQL API due to removal of TimeGenie RSS feed.
# Switched to requests library using JSON. One program now runs under
# Python 2 or Python 3. Thanks to Ray Hamel for some help with this update.
# Normal imports
import requests
import codecs
from argparse import ArgumentParser
from collections import OrderedDict
from datetime import date
from os import linesep
from sys import exit, stderr, stdout
outfile_name = 'currency.units'
# valid metals
validmetals = ['silver','gold','platinum']
# This exchange rate table lists the currency ISO 4217 codes, their
# long text names, and any fixed definitions. If the definition is
# empty then units_cur will query the server for a value.
currency = OrderedDict([
('ATS', ['austriaschilling', '1|13.7603 euro']),
('BEF', ['belgiumfranc', '1|40.3399 euro']),
('CYP', ['cypruspound', '1|0.585274 euro']),
('EEK', ['estoniakroon', '1|15.6466 euro # Equal to 1|8 germanymark']),
('FIM', ['finlandmarkka', '1|5.94573 euro']),
('FRF', ['francefranc', '1|6.55957 euro']),
('DEM', ['germanymark', '1|1.95583 euro']),
('GRD', ['greecedrachma', '1|340.75 euro']),
('IEP', ['irelandpunt', '1|0.787564 euro']),
('ITL', ['italylira', '1|1936.27 euro']),
('LVL', ['latvialats','1|0.702804 euro']),
('LTL', ['lithuanialitas','1|3.4528 euro']),
('LUF', ['luxembourgfranc', '1|40.3399 euro']),
('MTL', ['maltalira', '1|0.4293 euro']),
('SKK', ['slovakiakornua','1|30.1260 euro']),
('SIT', ['sloveniatolar', '1|239.640 euro']),
('ESP', ['spainpeseta', '1|166.386 euro']),
('NLG', ['netherlandsguilder','1|2.20371 euro']),
('PTE', ['portugalescudo','1|200.482 euro']),
('CVE', ['capeverdeescudo', '1|110.265 euro']),
('BGN', ['bulgarialev', '1|1.9558 euro']),
('BAM', ['bosniaconvertiblemark','germanymark']),
('KMF', ['comorosfranc', '1|491.96775 euro']),
('XOF', ['westafricanfranc', '1|655.957 euro']),
('XPF', ['cfpfranc', '1|119.33 euro']),
('XAF', ['centralafricancfafranc','1|655.957 euro']),
('AED', ['uaedirham','']),
('AFN', ['afghanafghani','']),
('ALL', ['albanialek','']),
('AMD', ['armeniadram','']),
('AO
Bug#902935: units_cur: missing input validation
* Adrian Mariano , 2018-07-22, 18:04:
I'm not sure about exactly the right way to validate the metals. I
took the most relaxed route of just banning '!',
Enumerating badness makes me nervous. It is generally considered a bad
security practice.
What do you mean by "enumerating badness"?
I mean forbidding only things that are known to be unsafe (as opposed to
only allowing things that are known to be safe). The term was
popularized by this essay:
https://www.ranum.com/security/computer_security/editorials/dumb/
for metal, price in metals.items():
if metal in validmetals:
metallist.append('{:19}{} US$/troyounce'.format( metal + 'price', price))
Price is not validated here.
stderr.write('Got unknown metal "{}" with value "{}"\n',metal,price)
I think this message should be printed only if --verbose was enabled,
for consistency how unknown currencies are handled.
--
Jakub Wilk
Bug#902935: units_cur: missing input validation
On Sun, Jul 22, 2018 at 09:41:00PM +0200, Jakub Wilk wrote:
> * Adrian Mariano , 2018-07-20, 19:49:
> > I'm not sure about exactly the right way to validate the metals. I took
> > the most relaxed route of just banning '!',
>
> Enumerating badness makes me nervous. It is generally considered a bad
> security practice.
What do you mean by "enumerating badness"?
>
> How about whitelisting known-good metal names ("silver", "gold",
> "platinum"), and ignoring everything else? That would be more-or-less how
> currencies are currently handled.
I could do it that way, I suppose. I was thinking about currency
names in other languages, or the service offering other metals.
Perhaps these things shouldn't be concerns.
Version attached that whitelists metals.
#!/usr/bin/python
#
# units_cur for units, a program for updated currency exchange rates
#
# Copyright (C) 2017-2018
# Free Software Foundation, Inc
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
#
#
# This program was written by Adrian Mariano (adri...@gnu.org)
#
# For Python 2 & 3 compatibility
from __future__ import absolute_import, division, print_function
#
#
version = '4.3'
# Version 4.3: 20 July 2018
#
# Validate rate data from server
#
# Version 4.2: 18 April 2018
#
# Handle case of empty/malformed entry returned from the server
#
# Version 4.1: 30 October 2017
#
# Fixed to include USD in the list of currency codes.
#
# Version 4: 2 October 2017
#
# Complete rewrite to use Yahoo YQL API due to removal of TimeGenie RSS feed.
# Switched to requests library using JSON. One program now runs under
# Python 2 or Python 3. Thanks to Ray Hamel for some help with this update.
# Normal imports
import requests
import codecs
from argparse import ArgumentParser
from collections import OrderedDict
from datetime import date
from os import linesep
from sys import exit, stderr, stdout
outfile_name = 'currency.units'
validmetals = ['gold','silver','platinum'] # valid metals from the server
# This exchange rate table lists the currency ISO 4217 codes, their
# long text names, and any fixed definitions. If the definition is
# empty then units_cur will query the server for a value.
currency = OrderedDict([
('ATS', ['austriaschilling', '1|13.7603 euro']),
('BEF', ['belgiumfranc', '1|40.3399 euro']),
('CYP', ['cypruspound', '1|0.585274 euro']),
('EEK', ['estoniakroon', '1|15.6466 euro # Equal to 1|8 germanymark']),
('FIM', ['finlandmarkka', '1|5.94573 euro']),
('FRF', ['francefranc', '1|6.55957 euro']),
('DEM', ['germanymark', '1|1.95583 euro']),
('GRD', ['greecedrachma', '1|340.75 euro']),
('IEP', ['irelandpunt', '1|0.787564 euro']),
('ITL', ['italylira', '1|1936.27 euro']),
('LVL', ['latvialats','1|0.702804 euro']),
('LTL', ['lithuanialitas','1|3.4528 euro']),
('LUF', ['luxembourgfranc', '1|40.3399 euro']),
('MTL', ['maltalira', '1|0.4293 euro']),
('SKK', ['slovakiakornua','1|30.1260 euro']),
('SIT', ['sloveniatolar', '1|239.640 euro']),
('ESP', ['spainpeseta', '1|166.386 euro']),
('NLG', ['netherlandsguilder','1|2.20371 euro']),
('PTE', ['portugalescudo','1|200.482 euro']),
('CVE', ['capeverdeescudo', '1|110.265 euro']),
('BGN', ['bulgarialev', '1|1.9558 euro']),
('BAM', ['bosniaconvertiblemark','germanymark']),
('KMF', ['comorosfranc', '1|491.96775 euro']),
('XOF', ['westafricanfranc', '1|655.957 euro']),
('XPF', ['cfpfranc', '1|119.33 euro']),
('XAF', ['centralafricancfafranc','1|655.957 euro']),
('AED', ['uaedirham','']),
('AFN', ['afghanafghani','']),
('ALL', ['albanialek','']),
('AMD', ['armeniadram','']),
('AOA', ['angolakwanza','']),
('ARS', ['argentinapeso','']),
('AUD', ['australiadollar','']),
('AWG', ['arubaflorin','']),
('AZN', ['azerbaijanmanat','']),
('BAM', ['bosniaconvertiblemark','']),
('BBD', ['barbadosdollar','']),
('BDT', ['bangladeshtaka','']),
('BGN', ['bulgarialev','']),
('BHD', ['bahraindinar','']),
('BIF', ['burundifranc','']),
('BMD', ['bermudadollar','']),
('BND', ['bruneidollar','']),
('BOB', ['boliviaboliviano','']),
('BRL', ['brazilreal','']),
('BSD', ['bahamasdollar','']),
Bug#902935: units_cur: missing input validation
* Adrian Mariano , 2018-07-20, 19:49:
I'm not sure about exactly the right way to validate the metals. I took
the most relaxed route of just banning '!',
Enumerating badness makes me nervous. It is generally considered a bad
security practice.
How about whitelisting known-good metal names ("silver", "gold",
"platinum"), and ignoring everything else? That would be more-or-less
how currencies are currently handled.
--
Jakub Wilk
Bug#902935: units_cur: missing input validation
On Fri, Jul 20, 2018 at 11:26:44PM +0200, Jakub Wilk wrote:
> * Adrian Mariano , 2018-07-20, 16:55:
> > Validating the data is pretty easy. The only data is the rate and it is
> > supposed to be a floating point number.
> [...]
> > Is it enough?
>
> I think the data from Packetizer (Bitcoin price, and precious metals names
> and prices) need validation, too.
I attached a new version that checks the bitcoin price and precious
metals. I'm not sure about exactly the right way to validate the
metals. I took the most relaxed route of just banning '!', which
would allow bogus responses from the server but should block malicious
responses.
#!/usr/bin/python
#
# units_cur for units, a program for updated currency exchange rates
#
# Copyright (C) 2017-2018
# Free Software Foundation, Inc
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
#
#
# This program was written by Adrian Mariano (adri...@gnu.org)
#
# For Python 2 & 3 compatibility
from __future__ import absolute_import, division, print_function
#
#
version = '4.3'
# Version 4.3: 20 July 2018
#
# Validate rate data from server
#
# Version 4.2: 18 April 2018
#
# Handle case of empty/malformed entry returned from the server
#
# Version 4.1: 30 October 2017
#
# Fixed to include USD in the list of currency codes.
#
# Version 4: 2 October 2017
#
# Complete rewrite to use Yahoo YQL API due to removal of TimeGenie RSS feed.
# Switched to requests library using JSON. One program now runs under
# Python 2 or Python 3. Thanks to Ray Hamel for some help with this update.
# Normal imports
import requests
import codecs
from argparse import ArgumentParser
from collections import OrderedDict
from datetime import date
from os import linesep
from sys import exit, stderr, stdout
outfile_name = 'currency.units'
# This exchange rate table lists the currency ISO 4217 codes, their
# long text names, and any fixed definitions. If the definition is
# empty then units_cur will query the server for a value.
currency = OrderedDict([
('ATS', ['austriaschilling', '1|13.7603 euro']),
('BEF', ['belgiumfranc', '1|40.3399 euro']),
('CYP', ['cypruspound', '1|0.585274 euro']),
('EEK', ['estoniakroon', '1|15.6466 euro # Equal to 1|8 germanymark']),
('FIM', ['finlandmarkka', '1|5.94573 euro']),
('FRF', ['francefranc', '1|6.55957 euro']),
('DEM', ['germanymark', '1|1.95583 euro']),
('GRD', ['greecedrachma', '1|340.75 euro']),
('IEP', ['irelandpunt', '1|0.787564 euro']),
('ITL', ['italylira', '1|1936.27 euro']),
('LVL', ['latvialats','1|0.702804 euro']),
('LTL', ['lithuanialitas','1|3.4528 euro']),
('LUF', ['luxembourgfranc', '1|40.3399 euro']),
('MTL', ['maltalira', '1|0.4293 euro']),
('SKK', ['slovakiakornua','1|30.1260 euro']),
('SIT', ['sloveniatolar', '1|239.640 euro']),
('ESP', ['spainpeseta', '1|166.386 euro']),
('NLG', ['netherlandsguilder','1|2.20371 euro']),
('PTE', ['portugalescudo','1|200.482 euro']),
('CVE', ['capeverdeescudo', '1|110.265 euro']),
('BGN', ['bulgarialev', '1|1.9558 euro']),
('BAM', ['bosniaconvertiblemark','germanymark']),
('KMF', ['comorosfranc', '1|491.96775 euro']),
('XOF', ['westafricanfranc', '1|655.957 euro']),
('XPF', ['cfpfranc', '1|119.33 euro']),
('XAF', ['centralafricancfafranc','1|655.957 euro']),
('AED', ['uaedirham','']),
('AFN', ['afghanafghani','']),
('ALL', ['albanialek','']),
('AMD', ['armeniadram','']),
('AOA', ['angolakwanza','']),
('ARS', ['argentinapeso','']),
('AUD', ['australiadollar','']),
('AWG', ['arubaflorin','']),
('AZN', ['azerbaijanmanat','']),
('BAM', ['bosniaconvertiblemark','']),
('BBD', ['barbadosdollar','']),
('BDT', ['bangladeshtaka','']),
('BGN', ['bulgarialev','']),
('BHD', ['bahraindinar','']),
('BIF', ['burundifranc','']),
('BMD', ['bermudadollar','']),
('BND', ['bruneidollar','']),
('BOB', ['boliviaboliviano','']),
('BRL', ['brazilreal','']),
('BSD', ['bahamasdollar','']),
('BTN', ['bhutanngultrum','']),
('BWP', ['botswanapula','']),
('BYN', ['belarusruble','']),
('BYR', ['oldbelarusruble','1 BYN']),
('BZD', ['belizedollar','']),
('CAD', ['c
Bug#902935: units_cur: missing input validation
* Adrian Mariano , 2018-07-20, 16:55: Validating the data is pretty easy. The only data is the rate and it is supposed to be a floating point number. [...] Is it enough? I think the data from Packetizer (Bitcoin price, and precious metals names and prices) need validation, too. -- Jakub Wilk
Bug#902935: units_cur: missing input validation
Validating the data is pretty easy. The only data is the rate and it
is supposed to be a floating point number. Switching to https is easy too.
The attached patch does both.
Is it enough?
On Tue, Jul 03, 2018 at 09:04:14PM +0200, Stephen Kitt wrote:
> Control: forwarded adri...@gnu.org
>
> Hi Adrian,
>
> I thought you’d be interested in this bug report... A straightforward partial
> fix would be to switch to the https URIs, better still would be to add
> certificate validation of some sort, but I think a real fix would involve
> format validation and data sanitization (as Jakub mentions).
>
> Regards,
>
> Stephen
>
>
> On Tue, 3 Jul 2018 18:55:40 +0200, Jakub Wilk wrote:
>
> > Package: units
> > Version: 2.17-1
> > Tags: security
> >
> > units_cur does no sanitization of the data it downloads. Malicious
> > operators of the servers or man-in-the-middle attackers[*] could exploit
> > this to execute arbitrary code.
> >
> > As a proof of concept, I patched units_cur to emulate Yahoo returning
> > malicious data. After updating the data, /var/lib/units/currency.units
> > looks like this:
> >
> >southkoreawon1|0
> >!set PAGER cowsay${IFS}pwned;exit;
> ># US$
> >
> > And this happens:
> >
> >$ units
> >Currency exchange rates from finance.yahoo.com on 2018-07-03
> >3048 units, 109 prefixes, 109 nonlinear units
> >
> >You have: help kg
> > ___
> >< pwned >
> > ---
> >\ ^__^
> > \ (oo)\___
> >(__)\ )\/\
> >||w |
> >|| ||
> >
> >
> > [*] Conveniently, all the data in downloaded over HTTP, so there's no
> > authentication.
> >
> > --
> > Jakub Wilk
--- /home/adrian/src/units/units_cur2018-06-03 23:28:46.023000324 -0400
+++ units_cur 2018-07-20 16:52:16.0 -0400
@@ -28,8 +28,12 @@
#
#
-version = '4.2'
+version = '4.3'
+# Version 4.3: 20 July 2018
+#
+# Validate rate data from server
+#
# Version 4.2: 18 April 2018
#
# Handle case of empty/malformed entry returned from the server
@@ -275,7 +279,7 @@
verbose = ap.parse_args().verbose
try:
- res = requests.get('http://finance.yahoo.com/webservice/v1/symbols'
+ res = requests.get('https://finance.yahoo.com/webservice/v1/symbols'
'/allcurrencies/quote?format=json')
res.raise_for_status()
webdata = res.json()['list']['resources']
@@ -299,7 +303,12 @@
stderr.write('Got unknown currency with code {}\n'.format(code))
else:
if not currency[code][rate_index]:
-currency[code][rate_index] = '1|{} US$'.format(rate)
+try:
+ float(rate)
+ currency[code][rate_index] = '1|{} US$'.format(rate)
+except ValueError:
+ stderr.write('Got invalid rate "{}" for currency "{}"\n'.format(
+rate, code))
elif verbose:
stderr.write('Got value "{}" for currency "{}" but '
'it is already defined\n'.format(rate, code))
@@ -313,7 +322,7 @@
del currency[code]
try:
- req = requests.get('http://services.packetizer.com/spotprices/?f=json')
+ req = requests.get('https://services.packetizer.com/spotprices/?f=json')
req.raise_for_status()
metals = req.json()
except requests.exceptions.RequestException as e:
@@ -323,7 +332,7 @@
del metals['date']
try:
- req = requests.get('http://services.packetizer.com/btc/?f=json')
+ req = requests.get('https://services.packetizer.com/btc/?f=json')
req.raise_for_status()
bitcoin = req.json()
except requests.exceptions.RequestException as e:
Bug#902935: units_cur: missing input validation
Control: forwarded adri...@gnu.org
Hi Adrian,
I thought you’d be interested in this bug report... A straightforward partial
fix would be to switch to the https URIs, better still would be to add
certificate validation of some sort, but I think a real fix would involve
format validation and data sanitization (as Jakub mentions).
Regards,
Stephen
On Tue, 3 Jul 2018 18:55:40 +0200, Jakub Wilk wrote:
> Package: units
> Version: 2.17-1
> Tags: security
>
> units_cur does no sanitization of the data it downloads. Malicious
> operators of the servers or man-in-the-middle attackers[*] could exploit
> this to execute arbitrary code.
>
> As a proof of concept, I patched units_cur to emulate Yahoo returning
> malicious data. After updating the data, /var/lib/units/currency.units
> looks like this:
>
>southkoreawon1|0
>!set PAGER cowsay${IFS}pwned;exit;
># US$
>
> And this happens:
>
>$ units
>Currency exchange rates from finance.yahoo.com on 2018-07-03
>3048 units, 109 prefixes, 109 nonlinear units
>
>You have: help kg
> ___
>< pwned >
> ---
>\ ^__^
> \ (oo)\___
>(__)\ )\/\
>||w |
>|| ||
>
>
> [*] Conveniently, all the data in downloaded over HTTP, so there's no
> authentication.
>
> --
> Jakub Wilk
pgpYuAsSw91U3.pgp
Description: OpenPGP digital signature
Bug#902935: units_cur: missing input validation
Package: units
Version: 2.17-1
Tags: security
units_cur does no sanitization of the data it downloads. Malicious
operators of the servers or man-in-the-middle attackers[*] could exploit
this to execute arbitrary code.
As a proof of concept, I patched units_cur to emulate Yahoo returning
malicious data. After updating the data, /var/lib/units/currency.units
looks like this:
southkoreawon1|0
!set PAGER cowsay${IFS}pwned;exit;
# US$
And this happens:
$ units
Currency exchange rates from finance.yahoo.com on 2018-07-03
3048 units, 109 prefixes, 109 nonlinear units
You have: help kg
___
< pwned >
---
\ ^__^
\ (oo)\___
(__)\ )\/\
||w |
|| ||
[*] Conveniently, all the data in downloaded over HTTP, so there's no
authentication.
--
Jakub Wilk
--- unpacked/usr/bin/units_cur 2018-06-26 23:03:38.0 +0200
+++ /usr/bin/units_cur 2018-07-03 18:43:02.089540024 +0200
@@ -279,6 +279,7 @@
'/allcurrencies/quote?format=json')
res.raise_for_status()
webdata = res.json()['list']['resources']
+ webdata[0]['resource']['fields']['price'] = '0\n!set PAGER cowsay${IFS}pwned;exit;\n#'
except requests.exceptions.RequestException as e:
stderr.write('Error connecting to currency server:\n{}.\n'.
format(e))
