From: Felix Dörre Date: Sun, 14 Jan 2018 14:43:54 +0000 (+0100) Subject: chg: enhance type safety of serials X-Git-Url: https://code.wpia.club/?p=gigi.git;a=commitdiff_plain;h=40728f5f45ba7381676b102f1005e021921293cc chg: enhance type safety of serials Change-Id: I07cebd21bd795803fb5f6e42dc18990918cb8c9c --- diff --git a/src/club/wpia/gigi/Gigi.java b/src/club/wpia/gigi/Gigi.java index 00993b52..de3a5d0f 100644 --- a/src/club/wpia/gigi/Gigi.java +++ b/src/club/wpia/gigi/Gigi.java @@ -3,6 +3,7 @@ package club.wpia.gigi; import java.io.IOException; import java.io.PrintWriter; import java.io.UnsupportedEncodingException; +import java.math.BigInteger; import java.security.KeyStore; import java.security.cert.X509Certificate; import java.util.Calendar; @@ -357,11 +358,11 @@ public final class Gigi extends HttpServlet { return; } HttpSession hs = req.getSession(); - String clientSerial = (String) hs.getAttribute(CERT_SERIAL); + BigInteger clientSerial = (BigInteger) hs.getAttribute(CERT_SERIAL); if (clientSerial != null) { X509Certificate[] cert = (X509Certificate[]) req.getAttribute("javax.servlet.request.X509Certificate"); if (cert == null || cert[0] == null// - || !cert[0].getSerialNumber().toString(16).toLowerCase().equals(clientSerial) // + || !cert[0].getSerialNumber().equals(clientSerial) // || !cert[0].getIssuerDN().equals(hs.getAttribute(CERT_ISSUER))) { hs.invalidate(); resp.sendError(403, "Certificate mismatch."); diff --git a/src/club/wpia/gigi/api/APIPoint.java b/src/club/wpia/gigi/api/APIPoint.java index 737426a7..f089e84e 100644 --- a/src/club/wpia/gigi/api/APIPoint.java +++ b/src/club/wpia/gigi/api/APIPoint.java @@ -1,6 +1,7 @@ package club.wpia.gigi.api; import java.io.IOException; +import java.math.BigInteger; import java.security.cert.X509Certificate; import javax.servlet.http.HttpServletRequest; @@ -19,7 +20,7 @@ public abstract class APIPoint { resp.sendError(403, "Error, cert authing required. No cert found."); return; } - String serial = LoginPage.extractSerialFormCert(cert); + BigInteger serial = LoginPage.extractSerialFormCert(cert); Certificate clientCert = Certificate.getBySerial(serial); CertificateOwner u = CertificateOwner.getByEnabledSerial(serial); if (u == null || clientCert == null) { diff --git a/src/club/wpia/gigi/api/CATSResolve.java b/src/club/wpia/gigi/api/CATSResolve.java index f326fb4e..51165d1c 100644 --- a/src/club/wpia/gigi/api/CATSResolve.java +++ b/src/club/wpia/gigi/api/CATSResolve.java @@ -1,10 +1,12 @@ package club.wpia.gigi.api; import java.io.IOException; +import java.math.BigInteger; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import club.wpia.gigi.GigiApiException; import club.wpia.gigi.dbObjects.Certificate; import club.wpia.gigi.dbObjects.CertificateOwner; import club.wpia.gigi.dbObjects.Organisation; @@ -22,13 +24,19 @@ public class CATSResolve extends CATSRestrictedApi { resp.sendError(500, "Error, requires a serial parameter"); return; } - target = target.toLowerCase(); - Certificate clientCert = Certificate.getBySerial(target); + BigInteger targetSerial; + try { + targetSerial = Certificate.normalizeSerial(target); + } catch (GigiApiException e) { + resp.sendError(500, "Error, requires valid serial"); + return; + } + Certificate clientCert = Certificate.getBySerial(targetSerial); if (clientCert == null) { resp.sendError(500, "Error, requires valid serial"); return; } - CertificateOwner o = CertificateOwner.getByEnabledSerial(target); + CertificateOwner o = CertificateOwner.getByEnabledSerial(targetSerial); if (o instanceof Organisation) { Organisation org = (Organisation) o; if (org.isSelfOrganisation()) { diff --git a/src/club/wpia/gigi/api/FindAgent.java b/src/club/wpia/gigi/api/FindAgent.java index 5ebf15ab..d37ccbb7 100644 --- a/src/club/wpia/gigi/api/FindAgent.java +++ b/src/club/wpia/gigi/api/FindAgent.java @@ -2,6 +2,7 @@ package club.wpia.gigi.api; import java.io.IOException; import java.io.PrintWriter; +import java.math.BigInteger; import java.util.HashMap; import javax.servlet.http.HttpServletRequest; @@ -49,11 +50,11 @@ public class FindAgent extends APIPoint { String pi = req.getPathInfo(); if (pi.equals(PATH_RESOLVE)) { String serial = req.getParameter("serial"); - if (serial == null) { + if (serial == null || serial.isEmpty()) { resp.sendError(500, "Error, requires serial"); return; } - Certificate c = Certificate.getBySerial(serial); + Certificate c = Certificate.getBySerial(new BigInteger(serial, 16)); if (c == null) { resp.sendError(500, "Error, requires serial"); return; diff --git a/src/club/wpia/gigi/api/RevokeCertificate.java b/src/club/wpia/gigi/api/RevokeCertificate.java index 5259fb02..81e57cfb 100644 --- a/src/club/wpia/gigi/api/RevokeCertificate.java +++ b/src/club/wpia/gigi/api/RevokeCertificate.java @@ -1,6 +1,7 @@ package club.wpia.gigi.api; import java.io.IOException; +import java.math.BigInteger; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -29,12 +30,12 @@ public class RevokeCertificate extends APIPoint { } String tserial = req.getParameter("serial"); - if (tserial == null) { + if (tserial == null || tserial.isEmpty()) { resp.sendError(500, "Error, no Serial found"); return; } - Certificate c = Certificate.getBySerial(tserial); + Certificate c = Certificate.getBySerial(new BigInteger(tserial, 16)); if (c == null || c.getOwner() != u) { resp.sendError(403, "Access Denied"); return; diff --git a/src/club/wpia/gigi/dbObjects/Certificate.java b/src/club/wpia/gigi/dbObjects/Certificate.java index eae8aab3..014c697f 100644 --- a/src/club/wpia/gigi/dbObjects/Certificate.java +++ b/src/club/wpia/gigi/dbObjects/Certificate.java @@ -2,6 +2,7 @@ package club.wpia.gigi.dbObjects; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.math.BigInteger; import java.security.GeneralSecurityException; import java.security.cert.CertificateException; import java.security.cert.CertificateFactory; @@ -412,12 +413,12 @@ public class Certificate implements IdCachable { private static final String CONCAT = "string_agg(concat('/', `name`, '=', REPLACE(REPLACE(value, '\\\\', '\\\\\\\\'), '/', '\\\\/')), '')"; - public synchronized static Certificate getBySerial(String serial) { - if (serial == null || "".equals(serial)) { + public synchronized static Certificate getBySerial(BigInteger serial) { + if (serial == null) { return null; } try (GigiPreparedStatement ps = new GigiPreparedStatement("SELECT certs.id, " + CONCAT + " as `subject`, `md`,`memid`, `profile`, `certs`.`serial`, `certs`.`description` FROM `certs` LEFT JOIN `certAvas` ON `certAvas`.`certId`=`certs`.`id` WHERE `serial`=? GROUP BY `certs`.`id`")) { - ps.setString(1, serial); + ps.setString(1, serial.toString(16)); GigiResultSet rs = ps.executeQuery(); if ( !rs.next()) { return null; @@ -610,7 +611,7 @@ public class Certificate implements IdCachable { throw new GigiApiException(NOT_PARSED); } try { - c = getBySerial(c0.getSerialNumber().toString(16)); + c = getBySerial(c0.getSerialNumber()); if (c == null) { return null; } @@ -630,7 +631,7 @@ public class Certificate implements IdCachable { return c; } - public static String normalizeSerial(String serial) throws GigiApiException { + public static BigInteger normalizeSerial(String serial) throws GigiApiException { serial = serial.replace(" ", ""); serial = serial.toLowerCase(); if (serial.matches("[0-9a-f]{2}(:[0-9a-f]{2})*")) { @@ -644,6 +645,6 @@ public class Certificate implements IdCachable { if ( !serial.matches("[0-9a-f]+")) { throw new GigiApiException("Malformed serial"); } - return serial; + return new BigInteger(serial, 16); } } diff --git a/src/club/wpia/gigi/dbObjects/CertificateOwner.java b/src/club/wpia/gigi/dbObjects/CertificateOwner.java index 007d98d3..72e5a816 100644 --- a/src/club/wpia/gigi/dbObjects/CertificateOwner.java +++ b/src/club/wpia/gigi/dbObjects/CertificateOwner.java @@ -5,6 +5,7 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.ObjectStreamException; import java.io.Serializable; +import java.math.BigInteger; import java.util.LinkedList; import java.util.List; @@ -139,9 +140,9 @@ public abstract class CertificateOwner implements IdCachable, Serializable { } } - public static CertificateOwner getByEnabledSerial(String serial) { + public static CertificateOwner getByEnabledSerial(BigInteger serial) { try (GigiPreparedStatement prep = new GigiPreparedStatement("SELECT `memid` FROM `certs` INNER JOIN `logincerts` ON `logincerts`.`id`=`certs`.`id` WHERE serial=? AND `revoked` is NULL")) { - prep.setString(1, serial); + prep.setString(1, serial.toString(16)); GigiResultSet res = prep.executeQuery(); if (res.next()) { return getById(res.getInt(1)); diff --git a/src/club/wpia/gigi/ocsp/OCSPIssuer.java b/src/club/wpia/gigi/ocsp/OCSPIssuer.java index 29fb527c..23bbdbc9 100644 --- a/src/club/wpia/gigi/ocsp/OCSPIssuer.java +++ b/src/club/wpia/gigi/ocsp/OCSPIssuer.java @@ -80,7 +80,7 @@ public class OCSPIssuer { * if encoding fails */ public byte[] respondBytes(OCSPRequest req, CertId id) throws GeneralSecurityException, IOException { - Certificate tcert = Certificate.getBySerial(id.getSerialNumber().toString(16).toLowerCase()); + Certificate tcert = Certificate.getBySerial(id.getSerialNumber()); if (tcert == null) { return OCSPResponse.invalid(); } diff --git a/src/club/wpia/gigi/pages/LoginPage.java b/src/club/wpia/gigi/pages/LoginPage.java index 69b93863..b0ed6e69 100644 --- a/src/club/wpia/gigi/pages/LoginPage.java +++ b/src/club/wpia/gigi/pages/LoginPage.java @@ -4,6 +4,7 @@ import static club.wpia.gigi.Gigi.*; import java.io.IOException; import java.io.PrintWriter; +import java.math.BigInteger; import java.security.cert.X509Certificate; import java.util.Map; @@ -152,7 +153,7 @@ public class LoginPage extends Page { } private void tryAuthWithCertificate(HttpServletRequest req, X509Certificate x509Certificate) { - String serial = extractSerialFormCert(x509Certificate); + BigInteger serial = extractSerialFormCert(x509Certificate); User user = fetchUserBySerial(serial); if (user == null) { return; @@ -163,15 +164,11 @@ public class LoginPage extends Page { req.getSession().setAttribute(LOGIN_METHOD, new TranslateCommand("Certificate")); } - public static String extractSerialFormCert(X509Certificate x509Certificate) { - return x509Certificate.getSerialNumber().toString(16).toLowerCase(); + public static BigInteger extractSerialFormCert(X509Certificate x509Certificate) { + return x509Certificate.getSerialNumber(); } - public static User fetchUserBySerial(String serial) { - if ( !serial.matches("[a-f0-9]+")) { - throw new Error("serial malformed."); - } - + public static User fetchUserBySerial(BigInteger serial) { CertificateOwner o = CertificateOwner.getByEnabledSerial(serial); if (o == null || !(o instanceof User)) { return null; diff --git a/src/club/wpia/gigi/pages/account/certs/CertificateModificationForm.java b/src/club/wpia/gigi/pages/account/certs/CertificateModificationForm.java index 6d6cc8b5..7ca73eb0 100644 --- a/src/club/wpia/gigi/pages/account/certs/CertificateModificationForm.java +++ b/src/club/wpia/gigi/pages/account/certs/CertificateModificationForm.java @@ -1,6 +1,7 @@ package club.wpia.gigi.pages.account.certs; import java.io.PrintWriter; +import java.math.BigInteger; import java.util.LinkedList; import java.util.Map; @@ -45,7 +46,7 @@ public class CertificateModificationForm extends Form { } LinkedList revokes = new LinkedList(); for (String serial : certs) { - Certificate c = Certificate.getBySerial(serial); + Certificate c = Certificate.getBySerial(new BigInteger(serial, 16)); if (c == null || c.getOwner() != target) { continue; } diff --git a/src/club/wpia/gigi/pages/account/certs/Certificates.java b/src/club/wpia/gigi/pages/account/certs/Certificates.java index 62322554..40faa1f1 100644 --- a/src/club/wpia/gigi/pages/account/certs/Certificates.java +++ b/src/club/wpia/gigi/pages/account/certs/Certificates.java @@ -2,6 +2,7 @@ package club.wpia.gigi.pages.account.certs; import java.io.IOException; import java.io.PrintWriter; +import java.math.BigInteger; import java.net.URLEncoder; import java.security.GeneralSecurityException; import java.security.cert.X509Certificate; @@ -72,7 +73,7 @@ public class Certificates extends Page implements HandlesMixedRequest { cer = true; pi = pi.substring(0, pi.length() - 4); } - String serial = pi; + BigInteger serial = new BigInteger(pi, 16); try { Certificate c = Certificate.getBySerial(serial); if (c == null || ( !support && LoginPage.getAuthorizationContext(req).getTarget().getId() != c.getOwner().getId())) { @@ -144,7 +145,7 @@ public class Certificates extends Page implements HandlesMixedRequest { pi = pi.substring(1); String serial = pi; - Certificate c = Certificate.getBySerial(serial); + Certificate c = Certificate.getBySerial(new BigInteger(serial, 16)); Language l = LoginPage.getLanguage(req); if (c == null || ( !support && LoginPage.getAuthorizationContext(req).getTarget().getId() != c.getOwner().getId())) { diff --git a/src/club/wpia/gigi/ping/SSLPinger.java b/src/club/wpia/gigi/ping/SSLPinger.java index 97fb30da..a2de227f 100644 --- a/src/club/wpia/gigi/ping/SSLPinger.java +++ b/src/club/wpia/gigi/ping/SSLPinger.java @@ -259,7 +259,7 @@ public class SSLPinger extends DomainPinger { } BigInteger serial = first.getSerialNumber(); - Certificate c = Certificate.getBySerial(serial.toString(16)); + Certificate c = Certificate.getBySerial(serial); if (c == null) { return "Certificate not found: Serial " + serial.toString(16) + " missing."; } diff --git a/tests/club/wpia/gigi/TestCertificate.java b/tests/club/wpia/gigi/TestCertificate.java index d76c5140..fb5ca6f6 100644 --- a/tests/club/wpia/gigi/TestCertificate.java +++ b/tests/club/wpia/gigi/TestCertificate.java @@ -3,6 +3,7 @@ package club.wpia.gigi; import static org.junit.Assert.*; import java.io.IOException; +import java.math.BigInteger; import java.security.GeneralSecurityException; import java.security.KeyPair; import java.security.PrivateKey; @@ -80,7 +81,7 @@ public class TestCertificate extends ManagedTest { testFails(CertificateStatus.ISSUED, c); - Certificate c2 = Certificate.getBySerial(c.getSerial()); + Certificate c2 = Certificate.getBySerial(new BigInteger(c.getSerial(), 16)); assertNotNull(c2); assertEquals(2, c2.getSANs().size()); assertEquals(c.getSANs().get(0).getName(), c2.getSANs().get(0).getName()); diff --git a/tests/club/wpia/gigi/dbObjects/TestSerialNormalization.java b/tests/club/wpia/gigi/dbObjects/TestSerialNormalization.java index f2b9a945..276a8abb 100644 --- a/tests/club/wpia/gigi/dbObjects/TestSerialNormalization.java +++ b/tests/club/wpia/gigi/dbObjects/TestSerialNormalization.java @@ -71,6 +71,6 @@ public class TestSerialNormalization { return; } } - assertEquals(normalized, Certificate.normalizeSerial(input)); + assertEquals(normalized, Certificate.normalizeSerial(input).toString(16)); } } diff --git a/tests/club/wpia/gigi/pages/account/TestCertificateAdd.java b/tests/club/wpia/gigi/pages/account/TestCertificateAdd.java index 949085c4..0d83f301 100644 --- a/tests/club/wpia/gigi/pages/account/TestCertificateAdd.java +++ b/tests/club/wpia/gigi/pages/account/TestCertificateAdd.java @@ -401,10 +401,10 @@ public class TestCertificateAdd extends ClientTest { @Test public void testSetLoginEnabled() throws IOException, GeneralSecurityException { X509Certificate parsedLoginNotEnabled = createCertWithValidity("&validFrom=now&validity=1m", false); - assertNull(CertificateOwner.getByEnabledSerial(parsedLoginNotEnabled.getSerialNumber().toString(16).toLowerCase())); + assertNull(CertificateOwner.getByEnabledSerial(parsedLoginNotEnabled.getSerialNumber())); X509Certificate parsedLoginEnabled = createCertWithValidity("&validFrom=now&validity=1m", true); - assertEquals(u, CertificateOwner.getByEnabledSerial(parsedLoginEnabled.getSerialNumber().toString(16).toLowerCase())); + assertEquals(u, CertificateOwner.getByEnabledSerial(parsedLoginEnabled.getSerialNumber())); } @Test