From 97f3d9e2c621c7f4c64a1a737eaca202938389be Mon Sep 17 00:00:00 2001 From: Alexander Bakker Date: Mon, 1 Jan 2018 22:14:11 +0100 Subject: [PATCH] Improve manual entry validation --- .../me/impy/aegis/EditProfileActivity.java | 31 +++-- .../java/me/impy/aegis/crypto/KeyInfo.java | 118 +++++++++++++----- .../impy/aegis/crypto/KeyInfoException.java | 20 +++ .../java/me/impy/aegis/encoding/Base32.java | 65 +++++----- .../impy/aegis/encoding/Base32Exception.java | 9 +- .../impy/aegis/importers/FreeOTPImporter.java | 4 +- 6 files changed, 173 insertions(+), 74 deletions(-) create mode 100644 app/src/main/java/me/impy/aegis/crypto/KeyInfoException.java diff --git a/app/src/main/java/me/impy/aegis/EditProfileActivity.java b/app/src/main/java/me/impy/aegis/EditProfileActivity.java index c9f82ae2..c6e2de4e 100644 --- a/app/src/main/java/me/impy/aegis/EditProfileActivity.java +++ b/app/src/main/java/me/impy/aegis/EditProfileActivity.java @@ -16,9 +16,12 @@ import android.widget.EditText; import android.widget.ImageView; import android.widget.Spinner; +import me.impy.aegis.crypto.CryptoUtils; import me.impy.aegis.crypto.KeyInfo; +import me.impy.aegis.crypto.KeyInfoException; import me.impy.aegis.db.DatabaseEntry; import me.impy.aegis.encoding.Base32; +import me.impy.aegis.helpers.AuthHelper; import me.impy.aegis.helpers.SpinnerHelper; public class EditProfileActivity extends AegisActivity { @@ -89,7 +92,8 @@ public class EditProfileActivity extends AegisActivity { byte[] secretBytes = entry.getInfo().getSecret(); if (secretBytes != null) { - _textSecret.setText(Base32.encodeOriginal(secretBytes)); + char[] secretChars = Base32.encode(secretBytes); + _textSecret.setText(secretChars, 0, secretChars.length); } String type = entry.getInfo().getType(); @@ -154,6 +158,11 @@ public class EditProfileActivity extends AegisActivity { } private boolean onSave() { + if (_textSecret.length() == 0) { + onError("Secret is a required field."); + return false; + } + int period; try { period = Integer.parseInt(_textPeriod.getText().toString()); @@ -176,12 +185,20 @@ public class EditProfileActivity extends AegisActivity { DatabaseEntry entry = _profile.getEntry(); entry.setName(_textName.getText().toString()); KeyInfo info = entry.getInfo(); - info.setIssuer(_textIssuer.getText().toString()); - info.setSecret(Base32.decode(_textSecret.getText().toString())); - info.setPeriod(period); - info.setDigits(digits); - info.setAlgorithm(algo); - info.setType(type); + + try { + char[] secret = AuthHelper.getEditTextChars(_textSecret); + info.setSecret(secret); + CryptoUtils.zero(secret); + info.setIssuer(_textIssuer.getText().toString()); + info.setPeriod(period); + info.setDigits(digits); + info.setAlgorithm(algo); + info.setType(type); + } catch (KeyInfoException e) { + onError("The entered info is incorrect: " + e.getMessage()); + return false; + } Intent intent = new Intent(); intent.putExtra("KeyProfile", _profile); diff --git a/app/src/main/java/me/impy/aegis/crypto/KeyInfo.java b/app/src/main/java/me/impy/aegis/crypto/KeyInfo.java index 14720ff0..4cb272ba 100644 --- a/app/src/main/java/me/impy/aegis/crypto/KeyInfo.java +++ b/app/src/main/java/me/impy/aegis/crypto/KeyInfo.java @@ -3,8 +3,10 @@ package me.impy.aegis.crypto; import android.net.Uri; import java.io.Serializable; +import java.util.Arrays; import me.impy.aegis.encoding.Base32; +import me.impy.aegis.encoding.Base32Exception; public class KeyInfo implements Serializable { private String _type = "totp"; @@ -24,7 +26,7 @@ public class KeyInfo implements Serializable { builder.appendQueryParameter("digits", Integer.toString(_digits)); builder.appendQueryParameter("period", Integer.toString(_period)); builder.appendQueryParameter("algorithm", _algorithm); - builder.appendQueryParameter("secret", Base32.encodeOriginal(_secret)); + builder.appendQueryParameter("secret", new String(Base32.encode(_secret))); if (_type.equals("hotp")) { builder.appendQueryParameter("counter", Long.toString(_counter)); } @@ -44,30 +46,25 @@ public class KeyInfo implements Serializable { return p - (System.currentTimeMillis() % p); } - public static KeyInfo fromURL(String s) throws Exception { + public static KeyInfo fromURL(String s) throws KeyInfoException { final Uri url = Uri.parse(s); if (!url.getScheme().equals("otpauth")) { - throw new Exception("unsupported protocol"); + throw new KeyInfoException("unsupported protocol"); } KeyInfo info = new KeyInfo(); - - // only 'totp' and 'hotp' are supported - info._type = url.getHost(); - if (info._type.equals("totp") && info._type.equals("hotp")) { - throw new Exception("unsupported type"); - } + info.setType(url.getHost()); // 'secret' is a required parameter String secret = url.getQueryParameter("secret"); if (secret == null) { - throw new Exception("'secret' is not set"); + throw new KeyInfoException("'secret' is not set"); } - info._secret = Base32.decode(secret); + info.setSecret(secret.toCharArray()); // provider info used to disambiguate accounts String path = url.getPath(); - String label = path != null ? path.substring(1) : ""; + String label = path != null && path.length() > 0 ? path.substring(1) : ""; if (label.contains(":")) { // a label can only contain one colon @@ -75,40 +72,40 @@ public class KeyInfo implements Serializable { String[] strings = label.split(":"); if (strings.length == 2) { - info._issuer = strings[0]; - info._accountName = strings[1]; + info.setIssuer(strings[0]); + info.setAccountName(strings[1]); } else { // at this point, just dump the whole thing into the accountName - info._accountName = label; + info.setAccountName(label); } } else { // label only contains the account name // grab the issuer's info from the 'issuer' parameter if it's present String issuer = url.getQueryParameter("issuer"); - info._issuer = issuer != null ? issuer : ""; - info._accountName = label; + info.setIssuer(issuer != null ? issuer : ""); + info.setAccountName(label); } // just use the defaults if these parameters aren't set String algorithm = url.getQueryParameter("algorithm"); if (algorithm != null) { - info._algorithm = algorithm; + info.setAlgorithm(algorithm); } String period = url.getQueryParameter("period"); if (period != null) { - info._period = Integer.parseInt(period); + info.setPeriod(Integer.parseInt(period)); } String digits = url.getQueryParameter("digits"); if (digits != null) { - info._digits = Integer.parseInt(digits); + info.setDigits(Integer.parseInt(digits)); } // 'counter' is required if the type is 'hotp' String counter = url.getQueryParameter("counter"); if (counter != null) { - info._counter = Long.parseLong(counter); - } else if (info._type.equals("hotp")) { - throw new Exception("'counter' was not set which is required for 'hotp'"); + info.setCounter(Long.parseLong(counter)); + } else if (info.getType().equals("hotp")) { + throw new KeyInfoException("'counter' was not set which is required for 'hotp'"); } return info; @@ -117,56 +114,119 @@ public class KeyInfo implements Serializable { public String getType() { return _type; } + public byte[] getSecret() { return _secret; } + public String getAccountName() { return _accountName; } + public String getIssuer() { return _issuer; } + public String getAlgorithm(boolean java) { if (java) { return "Hmac" + _algorithm; } return _algorithm; } + public int getDigits() { return _digits; } + public long getCounter() { return _counter; } + public int getPeriod() { return _period; } - public void setType(String type) { + public boolean isTypeValid(String type) { + return type.equals("totp") || type.equals("hotp"); + } + + public void setType(String type) throws KeyInfoException { + type = type.toLowerCase(); + if (!isTypeValid(type)) { + throw new KeyInfoException(String.format("unsupported otp type: %s", type)); + } _type = type.toLowerCase(); } + + public void setSecret(char[] base32) throws KeyInfoException { + byte[] secret; + try { + secret = Base32.decode(base32); + } catch (Base32Exception e) { + throw new KeyInfoException("bad secret", e); + } + + setSecret(secret); + } + public void setSecret(byte[] secret) { _secret = secret; } + public void setAccountName(String accountName) { _accountName = accountName; } + public void setIssuer(String issuer) { _issuer = issuer; } - public void setAlgorithm(String algorithm) { + + public boolean isAlgorithmValid(String algorithm) { + return algorithm.equals("SHA1") || algorithm.equals("SHA256") || algorithm.equals("SHA512"); + } + + public void setAlgorithm(String algorithm) throws KeyInfoException { if (algorithm.startsWith("Hmac")) { algorithm = algorithm.substring(4); } - _algorithm = algorithm.toUpperCase(); + algorithm = algorithm.toUpperCase(); + + if (!isAlgorithmValid(algorithm)) { + throw new KeyInfoException(String.format("unsupported algorithm: %s", algorithm)); + } + _algorithm = algorithm; } - public void setDigits(int digits) { + + public boolean isDigitsValid(int digits) { + return digits == 6 || digits == 8; + } + + public void setDigits(int digits) throws KeyInfoException { + if (!isDigitsValid(digits)) { + throw new KeyInfoException(String.format("unsupported amount of digits: %d", digits)); + } _digits = digits; } - public void setCounter(long count) { + + public boolean isCounterValid(long count) { + return count >= 0; + } + + public void setCounter(long count) throws KeyInfoException { + if (!isCounterValid(count)) { + throw new KeyInfoException(String.format("bad count: %d", count)); + } _counter = count; } - public void setPeriod(int period) { + + public boolean isPeriodValid(int period) { + return period > 0; + } + + public void setPeriod(int period) throws KeyInfoException { + if (!isPeriodValid(period)) { + throw new KeyInfoException(String.format("bad period: %d", period)); + } _period = period; } } diff --git a/app/src/main/java/me/impy/aegis/crypto/KeyInfoException.java b/app/src/main/java/me/impy/aegis/crypto/KeyInfoException.java new file mode 100644 index 00000000..13231b43 --- /dev/null +++ b/app/src/main/java/me/impy/aegis/crypto/KeyInfoException.java @@ -0,0 +1,20 @@ +package me.impy.aegis.crypto; + +public class KeyInfoException extends Exception { + public KeyInfoException(String message) { + super(message); + } + + public KeyInfoException(String message, Throwable cause) { + super(message, cause); + } + + @Override + public String getMessage() { + Throwable cause = getCause(); + if (cause == null) { + return super.getMessage(); + } + return String.format("%s (%s)", super.getMessage(), cause.getMessage()); + } +} diff --git a/app/src/main/java/me/impy/aegis/encoding/Base32.java b/app/src/main/java/me/impy/aegis/encoding/Base32.java index 4cb8dd42..1a07905c 100644 --- a/app/src/main/java/me/impy/aegis/encoding/Base32.java +++ b/app/src/main/java/me/impy/aegis/encoding/Base32.java @@ -1,5 +1,7 @@ package me.impy.aegis.encoding; +// modified for use in Aegis + /* (PD) 2001 The Bitzi Corporation * Please see http://bitzi.com/publicdomain for more info. * @@ -20,7 +22,9 @@ package me.impy.aegis.encoding; * limitations under the License. */ -import java.io.UnsupportedEncodingException; +import java.util.Arrays; + +import me.impy.aegis.crypto.CryptoUtils; /** * Base32 - encodes and decodes RFC3548 Base32 @@ -45,16 +49,6 @@ public class Base32 { 0x17,0x18,0x19,0xFF,0xFF,0xFF,0xFF,0xFF // 'x', 'y', 'z', '{', '|', '}', '~', 'DEL' }; - public static byte[] encode(byte[] data) throws UnsupportedEncodingException { - String lower = encodeOriginal(data).toLowerCase(); - return lower.getBytes("US-ASCII"); - } - - public static byte[] decodeModified(String data) { - return decode(data.replace('8', 'L').replace('9', 'O')); - } - - /** * Encodes byte array to Base32 String. * @@ -62,10 +56,10 @@ public class Base32 { * @return Encoded byte array bytes as a String. * */ - static public String encodeOriginal(final byte[] bytes) { - int i = 0, index = 0, digit = 0; + public static char[] encode(final byte[] bytes) { + int i = 0, index = 0, digit = 0, j = 0; int currByte, nextByte; - StringBuffer base32 = new StringBuffer((bytes.length + 7) * 8 / 5); + char[] base32 = new char[(bytes.length + 7) * 8 / 5]; while (i < bytes.length) { currByte = (bytes[i] >= 0) ? bytes[i] : (bytes[i] + 256); // unsign @@ -90,10 +84,12 @@ public class Base32 { if (index == 0) i++; } - base32.append(base32Chars.charAt(digit)); + base32[j++] = base32Chars.charAt(digit); } - return base32.toString(); + char[] res = Arrays.copyOf(base32, j); + CryptoUtils.zero(base32); + return res; } /** @@ -102,24 +98,13 @@ public class Base32 { * @param base32 * @return Decoded base32 String as a raw byte array. */ - static public byte[] decode(final String base32) { + public static byte[] decode(final char[] base32) throws Base32Exception { int i, index, lookup, offset, digit; - byte[] bytes = new byte[base32.length() * 5 / 8]; + byte[] bytes = new byte[base32.length * 5 / 8]; - for (i = 0, index = 0, offset = 0; i < base32.length(); i++) { - lookup = base32.charAt(i) - '0'; - - /* Skip chars outside the lookup table */ - if (lookup < 0 || lookup >= base32Lookup.length) { - continue; - } - - digit = base32Lookup[lookup]; - - /* If this digit is not in the table, ignore it */ - if (digit == 0xFF) { - continue; - } + for (i = 0, index = 0, offset = 0; i < base32.length; i++) { + lookup = base32[i] - '0'; + digit = decodeDigit(lookup); if (index <= 3) { index = (index + 5) % 8; @@ -144,4 +129,20 @@ public class Base32 { } return bytes; } + + private static int decodeDigit(int c) throws Base32Exception { + /* Skip chars outside the lookup table */ + if (c < 0 || c >= base32Lookup.length) { + throw new Base32Exception("char not found in base32 lookup table"); + } + + int digit = base32Lookup[c]; + + /* If this digit is not in the table, ignore it */ + if (digit == 0xFF) { + throw new Base32Exception("char not found in base32 lookup table"); + } + + return digit; + } } diff --git a/app/src/main/java/me/impy/aegis/encoding/Base32Exception.java b/app/src/main/java/me/impy/aegis/encoding/Base32Exception.java index 9ddb74e4..5924b5f8 100644 --- a/app/src/main/java/me/impy/aegis/encoding/Base32Exception.java +++ b/app/src/main/java/me/impy/aegis/encoding/Base32Exception.java @@ -1,8 +1,7 @@ package me.impy.aegis.encoding; -/** - * Created by alex on 1/1/18. - */ - -public class Base32Exception { +public class Base32Exception extends Exception { + public Base32Exception(String message) { + super(message); + } } diff --git a/app/src/main/java/me/impy/aegis/importers/FreeOTPImporter.java b/app/src/main/java/me/impy/aegis/importers/FreeOTPImporter.java index e74f9c2c..ec39fa2c 100644 --- a/app/src/main/java/me/impy/aegis/importers/FreeOTPImporter.java +++ b/app/src/main/java/me/impy/aegis/importers/FreeOTPImporter.java @@ -13,6 +13,7 @@ import java.util.ArrayList; import java.util.List; import me.impy.aegis.crypto.KeyInfo; +import me.impy.aegis.crypto.KeyInfoException; import me.impy.aegis.db.DatabaseEntry; import me.impy.aegis.util.ByteInputStream; @@ -40,7 +41,8 @@ public class FreeOTPImporter extends DatabaseImporter { return "FreeOTP"; } - private static List parse(XmlPullParser parser) throws IOException, XmlPullParserException, JSONException { + private static List parse(XmlPullParser parser) + throws IOException, XmlPullParserException, JSONException, KeyInfoException { List entries = new ArrayList<>(); parser.require(XmlPullParser.START_TAG, null, "map");