Meta tags:
description=CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security - Java: An experimental query for ignored hostname verification by artem-smotrakov · Pull Request #6443 · github/codeql;

Headings (most frequently used words):

uh, oh, commented, smotrakov, artem, 2022, feb, 2021, for, this, comment, nov, choose, reason, hiding, an, ignored, hostname, loading, aug, kevinbackhouse, jan, atorralba, 10, saved, searches, java, experimental, query, verification, navigation, to, footer, edited, there, was, error, while, please, reload, page, smowton, 12, 14, search, code, repositories, users, issues, pull, requests, provide, feedback, 6443, menu, use, filter, your, results, more, quickly, 6443smowton, merged, 13, commits, intogithub, maingithub, codeql, mainfrom, verifierartem, ql, verifiercopy, head, branch, name, clipboard, conversation, intrigus, lgtm, 26, 23, 16, 19, left, reviewers, assignees, labels, projects, milestone, development, participants,

Text of the page (most frequently used words):
the (87), this (78), there (41), artem (40), #smotrakov (40), was (38), while (35), please (34), reload (34), loading (33), all (33), error (31), page (31), query (30), for (26), #github (25), that (24), comment (24), sorry (24), not (23), copy (23), security (22), something (22), cwe (22), java (21), 2022 (21), went (21), wrong (21), reactions (21), contributor (21), link (21), hostname (21), resolved (20), ignored (20), and (19), code (17), you (16), feb (16), commented (16), with (16), ignoredhostnameverification (16), hostnameverifier (16), false (16), outdated (15), reason (15), experimental (15), lgtm (15), from (14), com (14), smowton (13), result (13), https (13), 2021 (13), applied (12), reaction (12), reacted (12), emoji (12), added (12), hide (12), method (12), 297 (12), positives (12), can (11), more (11), author (11), verifier (11), verification (11), suggestions (10), atorralba (10), view (10), your (10), choose (10), hiding (10), src (10), another (10), verify (10), think (10), pull (9), changes (9), like (9), are (9), suggestion (8), review (8), show (8), expression (8), hostnameverificationcall (8), call (8), qhelp (8), hostnameverifierverifymethod (8), kevinbackhouse (8), cannot (7), reviewed (7), thumbs (7), others (7), comments (6), may (6), what (6), learn (6), use (6), off (6), topic (6), will (6), problem (6), codeql (6), but (6), instanceof (6), would (6), results (6), assert (6), aug (6), enterprise (6), right (5), request (5), change (5), these (5), issues (5), documentation (5), sign (5), merged (5), main (5), child (5), isignored (5), commits (5), duplicate (5), abuse (5), spam (5), displayed (5), describe (5), eyes (5), used (5), extends (5), class (5), private (5), calls (5), wrapped (5), make (5), sense (5), look (5), still (5), fixed (5), nov (5), search (5), community (4), navigation (4), now (4), check (4), line (4), has (4), add (4), only (4), one (4), because (4), marcono1234 (4), file (4), about (4), into (4), direct (4), any (4), exprstmt (4), better (4), some (4), ifstmt (4), methodaccess (4), let (4), makes (4), sure (4), exception (4), updated (4), those (4), completely (4), number (4), branch (4), examples (4), tests (4), looking (4), quite (4), actions (4), checks (4), feedback (4), 6443 (4), support (4), manage (3), 2026 (3), batch (3), commit (3), projects (3), reviewers (3), left (3), conversation (3), hidden (3), characters (3), unicode (3), open (3), thanks (3), redundant (3), conditions (3), other (3), statement (3), exists (3), expr (3), where (3), predicate (3), getcaller (3), getmethod (3), defines (3), verity (3), socket (3), assigned (3), pretty (3), failed (3), see (3), might (3), very (3), when (3), jan (3), should (3), assertfalse (3), asserttrue (3), 1673043917935932127 (3), compare (3), positive (3), before (3), example (3), take (3), thank (3), intrigus (3), currently (3), filter (3), out (3), cases (3), value (3), returned (3), name (3), insights (3), models (3), requests (3), signed (3), settings (3), tab (3), window (3), refresh (3), session (3), saved (3), grade (3), features (3), platform (3), solutions (3), explore (3), share (2), footer (2), merge (2), been (2), must (2), create (2), lines (2), single (2), closed (2), development (2), milestone (2), labels (2), approved (2), already (2), have (2), bidirectional (2), removed (2), condition (2), semmle (2), encryption (2), clarify (2), authored (2), users (2), typos (2), also (2), unused (2), whether (2), them (2), hostnameverifierverify (2), hasstringsignature (2), string (2), sslsession (2), hasqualifiedname (2), javax (2), net (2), ssl (2), getasupertype (2), getdeclaringtype (2), clear (2), host (2), sslsocket (2), otherwise (2), know (2), they (2), cover (2), new (2), throw (2), fps (2), caused (2), being (2), best (2), pattern (2), afraid (2), large (2), which (2), warns (2), warn (2), variable (2), passed (2), argument (2), edited (2), cve (2), 2019 (2), 11777 (2), aware (2), test (2), database (2), january (2), 825fe17 (2), force (2), pushed (2), ignore (2), slightly (2), subtle (2), 6215280566352377624 (2), without (2), gone (2), down (2), seeing (2), lot (2), similar (2), vast (2), majority (2), involve (2), just (2), fixing (2), care (2), 6043668221389765828 (2), until (2), addressed (2), address (2), few (2), isn (2), working (2), here (2), alibaba (2), nacos (2), 8750717796235890424 (2), wouldn (2), inside (2), 1603409360070422690 (2), application (2), reports (2), server (2), connection (2), head (2), clipboard (2), discussions (2), appearance (2), cancel (2), available (2), searches (2), repositories (2), copilot (2), business (2), advanced (2), developer (2), topics (2), archive (2), source (2), resources (2), customer (2), services (2), devops (2), build (2), web (2), capture (2), mar (2), perform, action, time, personal, information, cookies, contact, docs, status, privacy, terms, inc, back, later, queued, multi, pending, reviews, marked, existing, order, valid, applying, deleted, supported, per, viewing, subset, invalid, were, made, participants, successfully, merging, close, none, yet, assignees, whose, approvals, affect, requirements, account, join, free, contains, text, interpreted, compiled, differently, than, appears, below, editor, reveals, fd4dc95, f2bc584, format, a62eae5, remove, 48604cd, 36e565d, classes, 651e43d, february, tony, torralba, noreply, chris, 0ba229a, apply, formatting, kinds, assignment, passing, conditional, nested, within, getexpr, however, determine, checking, getachildexpr, instead, relying, optimiser, compute, product, exprs, ifstmts, methodaccesses, disjuncts, each, scope, holds, import, suggested, reuse, defined, types, adding, least, object, likewise, boolean, successful, getsession, starthandshake, sslsocketfactory, getdefault, createsocket, port, checkedhostnameverification, inline, exotic, want, general, technically, f53b2fc, uses, trade, tps, could, great, ghsa, 63qc, p2x4, 9fgf, included, 84b5c47, 6dad0e2, vefify, dcf251b, 2b33265, f78002b, e11cb94, good, alerts, suppose, built, testing, mocking, frameworks, replying, slow, unfurtunately, delay, addressing, above, fixes, selfhostnameverifier, november, 60a0021, 5978475, task, securitylab, 419, mentioned, bot, requested, august, owner, team, detects, eclipse, paho, client, library, version, checked, requires, thrown, based, strict, failure, throwing, away, requirement, softened, evaluated, block, forward, certificate, matches, after, established, returns, true, acceptable, contract, does, require, therefore, caller, drop, files, changed, additional, options, 375, 933, star, fork, notification, notifications, public, message, dismiss, alert, switched, accounts, resetting, focus, qualifiers, our, quickly, submit, include, email, contacted, read, every, piece, input, seriously, provide, syntax, tips, jump, pricing, premium, ons, powered, collections, trending, program, stars, accelerator, maintainer, lab, programs, fund, developers, sponsors, partners, trust, center, forum, skills, ebooks, events, webinars, stories, type, software, industries, government, manufacturing, financial, healthcare, industry, devsecops, app, modernization, case, nonprofits, startups, small, medium, teams, enterprises, company, size, marketplace, changelog, blog, why, stop, leaks, start, secret, protection, secure, find, fix, vulnerabilities, plan, track, work, instant, dev, environments, codespaces, automate, workflow, workflows, integrate, external, tools, mcp, registry, prompts, deploy, intelligent, apps, spark, write, creation, toggle, menu, skip, content, wayback, machine, http, org, 20260322020851, timestamps, fail, success, 2027, 2025, apr,

Text of the page (random words):
eyes emoji all reactions 1 reaction 1 reaction sorry something went wrong uh oh there was an error while loading please reload this page artem smotrakov force pushed the ignored hostname verifier branch from 5978475 to 60a0021 compare november 7 2021 16 36 copy link contributor author artem smotrakov commented nov 7 2021 i ve addressed your comments marcono1234 thank you all reactions sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor author artem smotrakov commented nov 7 2021 wouldn t it make sense to filter out cases where the ignored value is returned from inside another hostnameverifier https lgtm com query 1603409360070422690 intrigus lgtm makes sense to me i ve updated the query to ignore calls wrapped by another hostname verifier thank you for the suggestion all reactions sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor author artem smotrakov commented nov 7 2021 artem smotrakov there are currently quite a few false positives because the ignored predicate isn t working quite right here are some examples on alibaba nacos https lgtm com query 8750717796235890424 kevinbackhouse i ve addressed intrigus lgtm s comment above and that fixes the false positive in selfhostnameverifier i am not sure that the query should be updated to address the false positives in tests please let me know what you think all reactions sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor author artem smotrakov commented nov 7 2021 thank you all for the review and sorry for the delay in addressing your comments all reactions sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor kevinbackhouse commented nov 23 2021 artem smotrakov i m sorry for not looking at this until now the number of false positives has gone down but i m still seeing a lot of similar results to before for example https lgtm com query 6043668221389765828 the vast majority of the false positives involve assert asserttrue or assert assertfalse so just fixing those would take care of about 90 of the false positives the others are slightly more subtle like these https lgtm com query 6215280566352377624 https lgtm com query 1673043917935932127 1 artem smotrakov reacted with thumbs up emoji 1 artem smotrakov reacted with eyes emoji all reactions 1 reaction 1 reaction sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor author artem smotrakov commented jan 9 2022 edited loading uh oh there was an error while loading please reload this page artem smotrakov i m sorry for not looking at this until now kevinbackhouse no problem i am also replying pretty slow unfurtunately the number of false positives has gone down but i m still seeing a lot of similar results to before for example https lgtm com query 6043668221389765828 the vast majority of the false positives involve assert asserttrue or assert assertfalse so just fixing those would take care of about 90 of the false positives those are alerts in tests i suppose the database should have been built without the test code do you think it makes sense to make the query aware of testing mocking frameworks the others are slightly more subtle like these https lgtm com query 6215280566352377624 that is a good one i ve fixed it https lgtm com query 1673043917935932127 i am looking into this one all reactions sorry something went wrong uh oh there was an error while loading please reload this page artem smotrakov added 6 commits january 16 2022 18 25 added a query for ignored hostname verification e11cb94 added ignoredhostnameverification ql added a qhelp file with examples added tests fixed a false positive in cwe 297 ignoredhostnameverification ql f78002b added a query for ignored hostname verification 2b33265 added ignoredhostnameverification ql added a qhelp file with examples added tests fixed typos in ignoredhostnameverification qhelp dcf251b ignore wrapped hostnameverifier vefify calls 6dad0e2 fixed another false positive in cwe 297 ignoredhostnameverification ql 825fe17 artem smotrakov force pushed the ignored hostname verifier branch from 84b5c47 to 825fe17 compare january 16 2022 18 56 copy link contributor author artem smotrakov commented jan 16 2022 https lgtm com query 1673043917935932127 kevinbackhouse i ve fixed this one i am still now sure that it is right to make the query aware of assert asserttrue and assert assertfalse i think test code should not be included in the database all reactions sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor kevinbackhouse commented jan 19 2022 artem smotrakov i m afraid there s still a very large number of results all of which look like false positives to me i think it might be better to change this query so that it only warns when the verify call is completely ignored i e do not warn if the result is used in an expression assigned to a variable or passed as an argument to a method call cve 2019 11777 was caused by the result being completely ignored so i think that would be the best pattern to look for all reactions sorry something went wrong uh oh there was an error while loading please reload this page copy link contributor author artem smotrakov commented feb 6 2022 edited loading uh oh there was an error while loading please reload this page i m afraid there s still a very large number of results all of which look like false positives to me i think it might be better to change this query so that it only warns when the verify call is completely ignored i e do not warn if the result is used in an expression assigned to a variable or passed as an argument to a method call ghsa 63qc p2x4 9fgf was caused by the result being completely ignored so i think that would be the best pattern to look for hi kevinbackhouse sure we can trade off tps for fps let me see what i can do if you could share those results that you think might be fps that would be great all reactions sorry something went wrong uh oh there was an error while loading please reload this page updated ignoredhostnameverification ql to cover more uses of hostname f53b2fc verifier verify copy link contributor author artem smotrakov commented feb 6 2022 kevinbackhouse i ve updated the query to check if the result of hostname verification is used in a general expression if statement and a method call technically the result may be used in other statement like while result throw new exception failed but they look pretty exotic so i am not sure if we want to cover them please let me know what you think all reactions sorry something went wrong uh oh there was an error while loading please reload this page smowton assigned atorralba feb 9 2022 atorralba reviewed feb 10 2022 view reviewed changes copy link contributor atorralba left a comment there was a problem hiding this comment choose a reason for hiding this comment the reason will be displayed to describe this comment to others learn more choose a reason spam abuse off topic outdated duplicate resolved hide comment i added some inline comments but otherwise lgtm sorry something went wrong uh oh there was an error while loading please reload this page all reactions java ql src experimental security cwe cwe 297 checkedhostnameverification java outdated 0 0 1 8 sslsocket socket sslsocket sslsocketfactory getdefault createsocket host port socket starthandshake boolean successful verifier verify host socket getsession copy link contributor atorralba feb 10 2022 there was a problem hiding this comment choose a reason for hiding this comment the reason will be displayed to describe this comment to others learn more choose a reason spam abuse off topic outdated duplicate resolved hide comment i think it would make sense to clarify what verifier is like adding a line that at least makes clear it s a hostnameverifier object likewise in ignoredhostnameverification java sorry something went wrong uh oh there was an error while loading please reload this page 1 artem smotrakov reacted with thumbs up emoji 1 artem smotrakov reacted with eyes emoji all reactions 1 reaction 1 reaction java ql src experimental security cwe cwe 297 ignoredhostnameverification qhelp outdated show resolved hide resolved uh oh there was an error while loading please reload this page java ql src experimental security cwe cwe 297 ignoredhostnameverification ql comment on lines 14 to 28 the hostnameverifier verify method private class hostnameverifierverifymethod extends method hostnameverifierverifymethod this getdeclaringtype getasupertype hasqualifiedname javax net ssl hostnameverifier and this hasstringsignature verify string sslsession defines hostnameverifier verity calls that is not wrapped in another hostnameverifier private class hostnameverificationcall extends methodaccess hostnameverificationcall this getmethod instanceof hostnameverifierverifymethod and not this getcaller instanceof hostnameverifierverifymethod copy link contributor atorralba feb 10 2022 there was a problem hiding this comment choose a reason for hiding this comment the reason will be displayed to describe this comment to others learn more choose a reason spam abuse off topic outdated duplicate resolved hide comment let s reuse already defined types suggested change the hostnameverifier verify method private class hostnameverifierverifymethod extends method hostnameverifierverifymethod this getdeclaringtype getasupertype hasqualifiedname javax net ssl hostnameverifier and this hasstringsignature verify string sslsession defines hostnameverifier verity calls that is not wrapped in another hostnameverifier private class hostnameverificationcall extends methodaccess hostnameverificationcall this getmethod instanceof hostnameverifierverifymethod and not this getcaller instanceof hostnameverifierverifymethod import semmle code java security encryption defines hostnameverifier verity calls that is not wrapped in another hostnameverifier private class hostnameverificationcall extends methodaccess hostnameverificationcall this getmethod instanceof hostnameverifierverify and not this getcaller instanceof hostnameverifierverify sorry something went wrong uh oh there was an error while loading please reload this page 1 artem smotrakov reacted with thumbs up emoji 1 artem smotrakov reacted with eyes emoji all reactions 1 reaction 1 reaction smowton reviewed feb 10 2022 view reviewed changes java ql src experimental security cwe cwe 297 ignoredhostnameverification qhelp outdated show resolved hide resolved uh oh there was an error while loading please reload this page java ql src experimental security cwe cwe 297 ignoredhostnameverification ql outdated show resolved hide resolved uh oh there was an error while loading please reload this page java ql src experimental security cwe cwe 297 ignoredhostnameverification ql outdated holds if the result of the call is not used predicate isignored not exists expr expr ifstmt ifstmt methodaccess ma copy link contributor smowton feb 10 2022 there was a problem hiding this comment choose a reason for hiding this comment the reason will be displayed to describe this comment to others learn more choose a reason spam abuse off topic outdated duplicate resolved hide comment this is relying on the codeql optimiser not to compute the product of all exprs ifstmts and methodaccesses because there are disjuncts where each of them are in scope but unused instead do something like not exists expr e this e getachildexpr and not exists ifstmt ifstmt however you can also determine if a method call is unused like this by checking whether it s the direct child of an exprstmt this any exprstmt es getexpr all other kinds of use assignment passing to a method call direct use in a conditional will be nested within some other statement or expression sorry something went wrong uh oh there was an error while loading please reload this page 1 artem smotrakov reacted with thumbs up emoji 1 artem smotrakov reacted with eyes emoji all reactions 1 reaction 1 reaction copy link contributor author artem smotrakov feb 12 2022 there was a problem hiding this comment choose a reason for hiding this comment the reason will be displayed to describe this comment to others learn more choose a reason spam abuse off topic outdated duplicate resolved hide comment thanks for the suggestions sorry something went wrong uh oh there was an error while loading please reload this page all reactions apply suggestions from code review typos formatting 0ba229a co authored by tony torralba atorralba users noreply github com co authored by chris smowton smowton github com artem smotrakov added 3 commits february 12 2022 12 24 clarify what verifier is 651e43d use classes from semmle code java security encryption 36e565d better hostnameverificationcall isignored 48604cd copy link contributor author artem smotrakov commented feb 12 2022 atorralba smowton thanks for the review i ve applied your suggestions all reactions sorry something went wrong uh oh there was an error while loading please reload this page remove redundant conditions from hostnameverificationcall isignored a62eae5 copy link contributor smowton commented feb 14 2022 i ve removed the redundant conditions from isignored an expression that is the direct child of an exprstmt can t be an if condition or be the child of any expression 1 artem smotrakov reacted with thumbs up emoji all reactions 1 reaction sorry something went wrong uh oh there was an error while loading please reload this page format f2bc584 copy link contributor author artem smotrakov commented feb 14 2022 i ve removed the redundant conditions from isignored an expression that is the direct child of an exprstmt can t be an if condition or be the child of any expression thanks all reactions sorry something went wrong uh oh there was an error while loading please reload this page smowton approved these changes feb 14 2022 view reviewed changes smowton merged commit fd4dc95 into github main feb 14 2022 this file contains hidden or bidirectional unicode text that may be interpreted or compiled differently than what appears below to review open the file in an editor that reveals hidden unicode characters learn more about bidirectional unicode characters show hidden characters sign up for free to join this conversation on github already have an account sign in to comment reviewers smowton smowton approved these changes 2 more reviewers atorralba atorralba left review comments marcono1234 marcono1234 left review comments reviewers whose approvals may not affect merge requirements assignees atorralba labels documentation java projects none yet milestone no milestone development successfully merging this pull request may close these issues uh oh there was an error while loading please reloa...