Merge branch 'main' into missing_flags

lib/src/compiler/ir/ast2ir.rs

@@ -438,7 +438,52 @@ pub(in crate::compiler) fn expr_from_ast(
         ast::Expr::BitwiseXor(expr) => bitwise_xor_expr_from_ast(ctx, expr),
 
         // Comparison operations
-        ast::Expr::Eq(expr) => eq_expr_from_ast(ctx, expr),
+        ast::Expr::Eq(expr) => {
+            let span = expr.span();
+
+            let lhs_span = expr.lhs.span();
+            let rhs_span = expr.rhs.span();
+
+            let lhs = expr_from_ast(ctx, &expr.lhs)?;
+            let rhs = expr_from_ast(ctx, &expr.rhs)?;
+
+            // Detect cases in which the equal operator is comparing a boolean
+            // expression with an integer constant (e.g: `pe.is_signed == 0`).
+            // This is quite common in YARA rules, it is accepted without
+            // errors, but a warning is raised.
+            let replacement = match (lhs.type_value(), rhs.type_value()) {
+                (TypeValue::Bool(_), TypeValue::Integer(Value::Const(0))) => {
+                    Some((
+                        Expr::Not { operand: Box::new(lhs) },
+                        format!("not {}", ctx.report_builder.get_snippet(&lhs_span.into()))
+                    ))
+                }
+                (TypeValue::Integer(Value::Const(0)), TypeValue::Bool(_)) => {
+                    Some((
+                        Expr::Not { operand: Box::new(rhs) },
+                        format!("not {}", ctx.report_builder.get_snippet(&rhs_span.into()))
+                    ))
+                }
+                (TypeValue::Bool(_), TypeValue::Integer(Value::Const(1))) => {
+                    Some((lhs, ctx.report_builder.get_snippet(&lhs_span.into())))
+                }
+                (TypeValue::Integer(Value::Const(1)), TypeValue::Bool(_)) => {
+                    Some((rhs, ctx.report_builder.get_snippet(&rhs_span.into())))
+                }
+                _ => None
+            };
+
+            if let Some((expr, msg)) = replacement {
+                ctx.warnings.add(|| Warning::boolean_integer_comparison(
+                    ctx.report_builder,
+                    span.into(),
+                    msg,
+                ));
+                Ok(expr)
+            } else {
+                eq_expr_from_ast(ctx, expr)
+            }
+        },
         ast::Expr::Ne(expr) => ne_expr_from_ast(ctx, expr),
         ast::Expr::Gt(expr) => gt_expr_from_ast(ctx, expr),
         ast::Expr::Ge(expr) => ge_expr_from_ast(ctx, expr),

lib/src/compiler/report.rs

@@ -137,6 +137,20 @@ impl ReportBuilder {
         self
     }
 
+    /// Returns the fragment of source code indicated by `source_ref`.
+    pub fn get_snippet(&self, source_ref: &SourceRef) -> String {
+        let source_id = source_ref
+            .source_id
+            .or_else(|| self.current_source_id())
+            .expect("create_report without registering any source code");
+
+        let cache = self.cache.borrow();
+        let cache_entry = cache.data.get(&source_id).unwrap();
+        let src = cache_entry.code.as_str();
+
+        src[source_ref.span.range()].to_string()
+    }
+
     /// Creates a new error or warning report.
     pub fn create_report(
         &self,

lib/src/compiler/tests/testdata/warnings/27.in

@@ -0,0 +1,5 @@
+import "test_proto2"
+
+rule test {
+	condition: test_proto2.array_bool[0] == 1
+}

lib/src/compiler/tests/testdata/warnings/27.out

@@ -0,0 +1,6 @@
+warning[bool_int_comparison]: comparison between boolean and integer
+ --> line:4:13
+  |
+4 |  condition: test_proto2.array_bool[0] == 1
+  |             ------------------------------ this comparison can be replaced with: `test_proto2.array_bool[0]`
+  |

lib/src/compiler/tests/testdata/warnings/28.in

@@ -0,0 +1,5 @@
+import "test_proto2"
+
+rule test {
+	condition: test_proto2.array_bool[0] == 0
+}

lib/src/compiler/tests/testdata/warnings/28.out

@@ -0,0 +1,6 @@
+warning[bool_int_comparison]: comparison between boolean and integer
+ --> line:4:13
+  |
+4 |  condition: test_proto2.array_bool[0] == 0
+  |             ------------------------------ this comparison can be replaced with: `not test_proto2.array_bool[0]`
+  |

lib/src/compiler/tests/testdata/warnings/29.in

@@ -0,0 +1,5 @@
+import "test_proto2"
+
+rule test {
+	condition: 1 == test_proto2.array_bool[0]
+}

lib/src/compiler/tests/testdata/warnings/29.out

@@ -0,0 +1,6 @@
+warning[bool_int_comparison]: comparison between boolean and integer
+ --> line:4:13
+  |
+4 |  condition: 1 == test_proto2.array_bool[0]
+  |             ------------------------------ this comparison can be replaced with: `test_proto2.array_bool[0]`
+  |

lib/src/compiler/tests/testdata/warnings/30.in

@@ -0,0 +1,5 @@
+import "test_proto2"
+
+rule test {
+	condition: 0 == test_proto2.array_bool[0]
+}

lib/src/compiler/tests/testdata/warnings/30.out

@@ -0,0 +1,6 @@
+warning[bool_int_comparison]: comparison between boolean and integer
+ --> line:4:13
+  |
+4 |  condition: 0 == test_proto2.array_bool[0]
+  |             ------------------------------ this comparison can be replaced with: `not test_proto2.array_bool[0]`
+  |

lib/src/compiler/warnings.rs

@@ -51,6 +51,14 @@ pub enum Warning {
         note: Option<String>,
     },
 
+    #[warning("bool_int_comparison", "comparison between boolean and integer")]
+    #[label("this comparison can be replaced with: `{replacement}`", span)]
+    BooleanIntegerComparison {
+        detailed_report: String,
+        span: SourceRef,
+        replacement: String,
+    },
+
     #[warning("duplicate_import", "duplicate import statement")]
     #[label(
       "duplicate import",

lib/src/modules/pe/authenticode.rs

@@ -22,6 +22,9 @@ use x509_parser::certificate::X509Certificate;
 use x509_parser::der_parser::num_bigint::BigUint;
 use x509_parser::x509::{AlgorithmIdentifier, SubjectPublicKeyInfo, X509Name};
 
+#[cfg(feature = "logging")]
+use log::error;
+
 use crate::modules::pe::asn1::{
     oid, oid_to_object_identifier, oid_to_str, Attribute, Certificate,
     ContentInfo, DigestInfo, SignedData, SignerInfo, SpcIndirectDataContent,
@@ -749,7 +752,11 @@ fn verify_message_digest(
         rfc5912::ID_MD_5 | rfc5912::MD_5_WITH_RSA_ENCRYPTION => {
             Md5::digest(message).as_slice() == digest
         }
-        _ => unimplemented!("{:?}", algorithm.oid()),
+        _ => {
+            #[cfg(feature = "logging")]
+            error!("unknown digest algorithm: {:?}", algorithm.oid());
+            false
+        }
     }
 }
 
@@ -884,8 +891,11 @@ fn verify_signer_info(si: &SignerInfo, certs: &[Certificate<'_>]) -> bool {
             attrs_set.write_der(&mut sha512).unwrap();
             key.verify_digest::<Sha512>(sha512.finalize(), si.signature)
         }
-
-        oid => unimplemented!("{:?}", oid),
+        _ => {
+            #[cfg(feature = "logging")]
+            error!("unknown digest algorithm: {:?}", digest_algorithm);
+            false
+        }
     }
 }
 
@@ -1143,7 +1153,14 @@ impl PublicKey {
             | rfc5912::ECDSA_WITH_SHA_512 => {
                 self.verify_impl::<Sha512>(message, signature)
             }
-            _ => unimplemented!("{:?}", digest_algorithm.oid()),
+            _ => {
+                #[cfg(feature = "logging")]
+                error!(
+                    "unknown digest algorithm: {:?}",
+                    digest_algorithm.oid()
+                );
+                false
+            }
         }
     }
 

lib/src/modules/test_proto2/tests/mod.rs

@@ -75,6 +75,11 @@ fn test_proto2_module() {
     condition_false!(r#"test_proto2.array_bool[0]"#);
     condition_true!(r#"test_proto2.array_bool[1]"#);
 
+    condition_false!(r#"test_proto2.array_bool[0] == 1"#);
+    condition_true!(r#"test_proto2.array_bool[0] == 0"#);
+    condition_false!(r#"1 == test_proto2.array_bool[0]"#);
+    condition_true!(r#"0 == test_proto2.array_bool[0]"#);
+
     // array_int64[3] is undefined, so both conditions are false.
     condition_false!(r#"test_proto2.array_int64[3] == 0"#);
     condition_false!(r#"test_proto2.array_int64[3] != 0"#);