Clever Geek Handbook

Licornes en sécurité: examen du code du château gonflable

image1.png


Voulez-vous voir un nouveau lot d'erreurs détectées par l'analyseur statique PVS-Studio pour Java? Alors rejoignez la lecture de l'article! Cette fois, le projet Bouncy Castle est devenu l'objet de vérification. Les extraits de code les plus intéressants, comme d'habitude, vous attendent ci-dessous.



Un peu sur PVS-Studio



PVS-Studio est un outil permettant d'identifier les erreurs et les vulnérabilités potentielles dans le code source des programmes. Au moment d'écrire ces lignes, l'analyse statique est implémentée pour les programmes écrits dans les langages de programmation C, C ++, C # et Java.



L'analyseur pour Java est la plus jeune gamme de PVS-Studio. Malgré cela, il n'est pas inférieur à ses frères aînés pour trouver des défauts dans le code. Cela est dû au fait que l'analyseur Java utilise toute la puissance des mécanismes de l'analyseur C ++. Vous pouvez en savoir plus sur cette union unique de Java et C ++ ici .



Pour le moment, il existe des plugins pour Gradle, Maven et IntelliJ IDEA pour une utilisation plus pratique . Si vous connaissez la plate-forme de contrôle de qualité en continu SonarQube, l'idée de jouer avecintégration du résultat de l'analyse.



Un peu sur le château gonflable



Bouncy Castle est un package avec l'implémentation d'algorithmes cryptographiques, écrits en langage de programmation Java (il existe aussi une implémentation en C #, mais cet article n'en parlera pas). Cette bibliothèque complète l'extension cryptographique standard (JCE) et contient une API adaptée à une utilisation dans n'importe quel environnement (y compris J2ME).



Les programmeurs sont libres d'utiliser toutes les fonctionnalités de cette bibliothèque. La mise en œuvre d'un grand nombre d'algorithmes et de protocoles rend ce projet très intéressant pour les développeurs de logiciels utilisant la cryptographie.



Commençons à vérifier



Bouncy Castle est un projet assez sérieux, car toute erreur dans une telle bibliothèque peut réduire la fiabilité du système de cryptage. Par conséquent, au début, nous avons même douté que nous puissions trouver au moins quelque chose d'intéressant dans cette bibliothèque, ou si toutes les erreurs avaient déjà été trouvées et corrigées avant nous. Disons tout de suite que notre analyseur Java ne nous a pas déçus :)



Naturellement, nous ne pouvons pas décrire tous les avertissements de l'analyseur en un seul article, mais nous avons une licence gratuite pour les développeurs qui développent des projets open source . Si vous le souhaitez, vous pouvez nous demander cette licence et analyser indépendamment le projet à l'aide de PVS-Studio.



Et nous allons commencer à regarder les extraits de code les plus intéressants qui ont été découverts.



Code inaccessible



V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. XMSSTest.java (170) 



public void testSignSHA256CompleteEvenHeight2() {
    ....
    int height = 10;
    ....
    for (int i = 0; i < (1 << height); i++) {
        byte[] signature = xmss.sign(new byte[1024]);
        switch (i) {
            case 0x005b:
                assertEquals(signatures[0], Hex.toHexString(signature));
                break;
            case 0x0822:
                assertEquals(signatures[1], Hex.toHexString(signature));
                break;
            ....
        }
    }
}


La valeur de la variable de hauteur dans la méthode ne change pas, donc le compteur i dans la boucle for ne peut pas être supérieur à 1024 (1 << 10). Cependant, dans l' instruction switch, le second cas vérifie i par rapport à la valeur 0x0822 (2082). Naturellement, l'octet de signatures [1] ne sera jamais vérifié .



Puisqu'il s'agit d'un code de méthode de test, il n'y a rien à craindre. C'est juste que les développeurs doivent faire attention au fait que le test de l'un des octets n'est jamais effectué.



Sous-expressions identiques



V6001 Il existe des sous-expressions identiques 'tag == PacketTags.SECRET_KEY' à gauche et à droite de '||' opérateur. PGPUtil.java (212), PGPUtil.java (212) 



public static boolean isKeyRing(byte[] blob) throws IOException {

    BCPGInputStream bIn = new BCPGInputStream(new ByteArrayInputStream(blob));
    int tag = bIn.nextPacketTag();

    return tag == PacketTags.PUBLIC_KEY || tag == PacketTags.PUBLIC_SUBKEY
        || tag == PacketTags.SECRET_KEY || tag == PacketTags.SECRET_KEY;
}


Dans cet extrait de code, l'instruction en retour , balise vérifiée deux fois == PacketTags.SECRET_KEY . Semblable à la vérification de la clé publique, la dernière vérification doit porter sur l'égalité entre la balise et PacketTags.SECRET_SUBKEY .



Code identique dans if / else



V6004 L' instruction «then» équivaut à l'instruction «else». BcAsymmetricKeyUnwrapper.java (36), BcAsymmetricKeyUnwrapper.java (40) 



public GenericKey generateUnwrappedKey(....) throws OperatorException {
    ....
    byte[] key = keyCipher.processBlock(....);
    if (encryptedKeyAlgorithm.getAlgorithm().equals(....)) {
        return new GenericKey(encryptedKeyAlgorithm, key);
    } else {
        return new GenericKey(encryptedKeyAlgorithm, key);
    }
}


Dans cet exemple, la méthode renvoie les mêmes instances de la classe GenericKey , que la condition du if soit satisfaite ou non. Il est clair que le code dans les branches if / else doit être différent, sinon la vérification dans if n'a pas du tout de sens. Ici, le programmeur a clairement été déçu par le copier-coller.



L'expression est toujours fausse



L' expression V6007 '! (NGroups <8)' est toujours fausse. CBZip2OutputStream.java (753) 



private void sendMTFValues() throws IOException {
    ....
    int nGroups;
    ....
    if (nMTF < 200) {
        nGroups = 2;
    } else if (nMTF < 600) {
        nGroups = 3;
    } else if (nMTF < 1200) {
        nGroups = 4;
    } else if (nMTF < 2400) {
        nGroups = 5;
    } else {
        nGroups = 6;
    }
    ....
    if (!(nGroups < 8)) {
        panic();
    }
}


Ici, la variable nGroups dans les blocs de code if / else reçoit une valeur qui est utilisée mais qui ne change nulle part. L'expression dans l' instruction if sera toujours fausse, car toutes les valeurs possibles pour nGroups : 2, 3, 4, 5 et 6 sont inférieures à 8.



L'analyseur comprend que la méthode panic () ne sera jamais exécutée et déclenche donc l'alarme. Mais ici, très probablement, la "programmation défensive" a été utilisée, et il n'y a rien à craindre.



Ajouter les mêmes éléments



V6033 Un élément avec la même clé 'PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC' a déjà été ajouté. PKCS12PBEUtils.java (50), PKCS12PBEUtils.java (49) 



class PKCS12PBEUtils {

    static {
        ....
        keySizes.put(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC,
                     Integers.valueOf(192));
        keySizes.put(PKCSObjectIdentifiers.pbeWithSHAAnd2_KeyTripleDES_CBC,
                     Integers.valueOf(128));
        ....
        desAlgs.add(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC);
        desAlgs.add(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC);
    }
}


Cette erreur est à nouveau due au copier-coller. Deux éléments identiques sont ajoutés au conteneur desAlgs . Le développeur a copié la dernière ligne du code, mais a oublié de corriger le chiffre 3 par 2 dans le nom du champ.



Index hors de portée



V6025 L' index «i» est peut-être hors limites. HSSTests.java (384) 



public void testVectorsFromReference() throws Exception {
    List<LMSigParameters> lmsParameters = new ArrayList<LMSigParameters>();
    List<LMOtsParameters> lmOtsParameters = new ArrayList<LMOtsParameters>();
    ....
    for (String line : lines) {        
        ....
        if (line.startsWith("Depth:")) {
            ....
        } else if (line.startsWith("LMType:")) {
            ....
            lmsParameters.add(LMSigParameters.getParametersForType(typ));
        } else if (line.startsWith("LMOtsType:")) {
            ....
            lmOtsParameters.add(LMOtsParameters.getParametersForType(typ));
        }
    }
    ....
    for (int i = 0; i != lmsParameters.size(); i++) {
        lmsParams.add(new LMSParameters(lmsParameters.get(i),
                                        lmOtsParameters.get(i)));
    }
}


L'ajout d'éléments à la collection lmsParameters et lmOtsParameters se fait dans la première boucle for , dans différentes branches de l' instruction if / else . Ensuite, dans la deuxième boucle for, les éléments de collection sont accédés à l'index i . Il vérifie uniquement que l'index i est inférieur à la taille de la première collection et que la taille de la deuxième collection n'est pas vérifiée dans la boucle for . Si les tailles des collections s'avèrent différentes, il est probable que vous puissiez obtenir une IndexOutOfBoundsException... Cependant, il convient de noter qu'il s'agit d'un code de méthode de test, et cet avertissement ne présente pas de danger particulier, car les collections sont remplies de données de test à partir d'un fichier pré-créé et, bien sûr, après l'ajout d'éléments, les collections ont la même taille.



Utilisation avant vérification nulle



V6060 La référence 'params' a été utilisée avant d'être vérifiée par rapport à null. BCDSAPublicKey.java (54), BCDSAPublicKey.java (53) 



BCDSAPublicKey(DSAPublicKeyParameters params) {
    this.y = params.getY();
    if (params != null) {
        this.dsaSpec = new DSAParameterSpec(params.getParameters().getP(),
                                            params.getParameters().getQ(),
                                            params.getParameters().getG());
    } else {
        this.dsaSpec = null;
    }
    this.lwKeyParams = params;
}


La première ligne de la méthode définit y sur params.getY () . Immédiatement après l'affectation, la variable params est vérifiée pour null . S'il est permis que les paramètres puissent être nuls dans cette méthode , vous devriez avoir effectué cette vérification avant d'utiliser la variable.



Enregistrement redondant si / sinon



V6003 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il existe une probabilité de présence d'erreur logique. EnrollExample.java (108), EnrollExample.java (113) 



public EnrollExample(String[] args) throws Exception {
    ....
    for (int t = 0; t < args.length; t++) {
        String arg = args[t];
        if (arg.equals("-r")) {
            reEnroll = true;
        } ....
        else if (arg.equals("--keyStoreType")) {
            keyStoreType = ExampleUtils.nextArgAsString
                           ("Keystore type", args, t);
            t += 1;
        } else if (arg.equals("--keyStoreType")) {
            keyStoreType = ExampleUtils.nextArgAsString
                           ("Keystore type", args, t);
            t += 1;
        } ....
    }
}


Dans l' instruction if / else, la valeur de la chaîne args est vérifiée deux fois pour l'égalité avec la chaîne "- keyStoreType ". Naturellement, le deuxième contrôle est redondant et n'a aucun sens. Cependant, cela ne ressemble pas à une erreur, car il n'y a aucun autre paramètre dans le texte d'aide de l'argument de ligne de commande qui ne soit traité dans le bloc if / else . Il s'agit probablement d'un code redondant qui devrait être supprimé.



La méthode renvoie la même valeur



V6014 Il est étrange que cette méthode renvoie toujours une seule et même valeur. XMSSSigner.java (129) 



public AsymmetricKeyParameter getUpdatedPrivateKey() {
    // if we've generated a signature return the last private key generated
    // if we've only initialised leave it in place
    // and return the next one instead.
    synchronized (privateKey) {
        if (hasGenerated) {
            XMSSPrivateKeyParameters privKey = privateKey;
            privateKey = null;
            return privKey;
        } else {
            XMSSPrivateKeyParameters privKey = privateKey;
            if (privKey != null) {
                privateKey = privateKey.getNextKey();
            }
            return privKey;
        }
    }
}


L'analyseur émet un avertissement car cette méthode renvoie toujours la même chose. Le commentaire de la méthode indique que selon que la signature a été générée ou non, la dernière clé générée doit être retournée ou la suivante. Apparemment, cette méthode a semblé suspecte à l'analyseur pour une raison.



Résumons



Comme vous pouvez le voir, nous avons quand même réussi à trouver des erreurs dans le projet Bouncy Castle, et cela ne fait que confirmer une fois de plus qu'aucun de nous n'écrit de code parfait. Différents facteurs peuvent fonctionner: le développeur est fatigué, inattentif, quelqu'un l'a distrait ... Tant que le code est écrit par une personne, des erreurs se produiront toujours.



Au fur et à mesure que les projets se développent, trouver des problèmes dans le code devient de plus en plus difficile. Par conséquent, dans le monde moderne, l'analyse de code statique n'est pas simplement une autre méthodologie, mais une réelle nécessité. Même si vous utilisez la révision de code, le TDD ou l'analyse dynamique, cela ne signifie pas que vous pouvez refuser l'analyse statique. Ce sont des méthodologies complètement différentes qui se complètent parfaitement.



En faisant de l'analyse statique l'une des étapes de développement, vous avez la possibilité de trouver des erreurs presque immédiatement, au moment de l'écriture du code. En conséquence, les développeurs n'auront pas besoin de passer des heures à déboguer ces erreurs. Les testeurs devront reproduire beaucoup moins de bogues insaisissables. Les utilisateurs obtiendront un programme qui fonctionne de manière plus fiable et plus stable.



En général, veillez à utiliser l'analyse statique dans vos projets! Nous le faisons nous-mêmes, et nous vous le recommandons :)





Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Irina Polynkina. Licornes en garde pour votre sécurité: découverte du code du château gonflable .


More articles:


All Articles