Skip to content

Commit

Permalink
handle missing dns-packet and user-flags
Browse files Browse the repository at this point in the history 
For reasons not yet known, userBlocklistFlagUint may go missing, and that
causes issues in dns-operation which don't at all expect it (null, undefined).

Fix that by treating empty, null, undefined strings as just empty, and
returning early from functions that can't do much with missing user-flags.

In a similar issue seen in prod, current-request doesn't do well with a missing
(undefined, null) decodedDnsPacket obj. So, before using it in current-request
make sure it is set to an equivalent empty obj.
  • Loading branch information
@ignoramous
ignoramous committed Dec 24, 2021
1 parent f2b3439 commit bc6dc50
Show file tree
Hide file tree
Showing 8 changed files with 54 additions and 18 deletions.
5 changes: 3 additions & 2 deletions src/basic/userOperation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
import { LocalCache as LocalCache } from "../cache-wrapper/cache-wrapper.js";
import { BlocklistFilter } from "../blocklist-wrapper/blocklistWrapper.js";
import * as util from "../helpers/util.js";

export class UserOperation {
constructor() {
Expand All @@ -23,7 +24,7 @@ export class UserOperation {
async RethinkModule(param) {
return this.loadUser(param);
}

loadUser(param) {
let response = {};
response.isException = false;
Expand Down Expand Up @@ -57,7 +58,7 @@ export class UserOperation {
currentUser.userBlocklistFlagUint = response.userBlocklistFlagUint;
currentUser.flagVersion = response.flagVersion;

if (currentUser.userBlocklistFlagUint !== "") {
if (!util.emptyString(currentUser.userBlocklistFlagUint)) {
currentUser.userServiceListUint = this.blocklistFilter
.flagIntersection(
currentUser.userBlocklistFlagUint,
Expand Down
9 changes: 4 additions & 5 deletions src/blocklist-wrapper/blocklistFilter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import { Buffer } from "buffer";
import { LocalCache } from "../cache-wrapper/cache-wrapper.js";
import { customTagToFlag as _customTagToFlag } from "./radixTrie.js";

import * as util from "../helpers/util.js";
import { base32, rbase32 } from "./b32.js";

export class BlocklistFilter {
Expand Down Expand Up @@ -83,12 +83,14 @@ export class BlocklistFilter {
}

flagIntersection(flag1, flag2) {
// handle empty flags
if (util.emptyString(flag1) || util.emptyString(flag2)) return false;

try {
let flag1Header = flag1[0];
let flag2Header = flag2[0];
let intersectHeader = flag1Header & flag2Header;
if (intersectHeader == 0) {
//console.log("first return")
return false;
}
let flag1Length = flag1.length - 1;
Expand All @@ -101,7 +103,6 @@ export class BlocklistFilter {
if ((flag1Header & 1) == 1) {
if ((tmpInterectHeader & 1) == 1) {
tmpBodyIntersect = flag1[flag1Length] & flag2[flag2Length];
//console.log(flag1[flag1Length] + " :&: " + flag2[flag2Length] + " -- " + tmpBodyIntersect)
if (tmpBodyIntersect == 0) {
intersectHeader = intersectHeader ^ maskHeaderForBodyEmpty;
} else {
Expand All @@ -118,9 +119,7 @@ export class BlocklistFilter {
flag2Header = flag2Header >>> 1;
maskHeaderForBodyEmpty = maskHeaderForBodyEmpty * 2;
}
//console.log(intersectBody)
if (intersectHeader == 0) {
//console.log("Second Return")
return false;
}
const intersectFlag = new Uint16Array(intersectBody.length + 1);
Expand Down
3 changes: 2 additions & 1 deletion src/dns-operation/dnsAggCache.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import DNSParserWrap from "./dnsParserWrap.js";
import DNSBlockOperation from "./dnsBlockOperation.js";
import { BlocklistFilter } from "../blocklist-wrapper/blocklistWrapper.js";
import * as util from "../helpers/util.js";

export default class DNSAggCache {
constructor() {
Expand Down Expand Up @@ -102,7 +103,7 @@ async function parseCacheapiResponse(
reqDecodedDnsPacket.questions[0].type == "HTTPS" ||
reqDecodedDnsPacket.questions[0].type == "SVCB") &&
metaData.blocklistInfo &&
userBlocklistInfo.userBlocklistFlagUint !== ""
!util.emptyString(userBlocklistInfo.userBlocklistFlagUint)
) {
metaData.blocklistInfo = new Map(Object.entries(metaData.blocklistInfo));
let blockResponse = dnsBlockOperation.checkDomainBlocking(
Expand Down
10 changes: 8 additions & 2 deletions src/dns-operation/dnsBlock.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import DNSParserWrap from "./dnsParserWrap.js";
import DNSBlockOperation from "./dnsBlockOperation.js";
import * as util from "../helpers/util.js";

export default class DNSBlock {
constructor() {
Expand All @@ -34,9 +35,8 @@ export default class DNSBlock {
response.data.isBlocked = false;
response.data.blockedB64Flag = "";
try {
if (param.userBlocklistInfo.userBlocklistFlagUint.length !== "") {
if (hasBlocklistStamp(param)) {
let domainNameBlocklistInfo;
// FIXME: handle HTTPS/SVCB
if (
(param.requestDecodedDnsPacket.questions.length >= 1) &&
(param.requestDecodedDnsPacket.questions[0].type == "A" ||
Expand Down Expand Up @@ -77,6 +77,12 @@ export default class DNSBlock {
}
}

function hasBlocklistStamp(param) {
return param &&
param.userBlocklistInfo &&
!util.emptyString(param.userBlocklistInfo.userBlocklistFlagUint);
}

function toCacheApi(param, wCache, domainNameBlocklistInfo) {
const dn =
param.requestDecodedDnsPacket.questions[0].name.trim().toLowerCase() + ":" +
Expand Down
2 changes: 1 addition & 1 deletion src/dns-operation/dnsBlockOperation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,4 @@ function checkDomainNameUserFlagIntersection(
throw e;
}
return response;
}
}
12 changes: 11 additions & 1 deletion src/dns-operation/dnsResponseBlock.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
*/

import DNSBlockOperation from "./dnsBlockOperation.js";
import * as util from "../helpers/util.js";

export default class DNSResponseBlock {
constructor() {
this.dnsBlockOperation = new DNSBlockOperation();
}

/**
* @param {*} param
* @param {*} param.userBlocklistInfo
Expand All @@ -28,7 +30,7 @@ export default class DNSResponseBlock {
response.data.isBlocked = false;
response.data.blockedB64Flag = "";
try {
if (param.userBlocklistInfo.userBlocklistFlagUint !== "") {
if (hasBlocklistStamp(param)) {
if (
param.responseDecodedDnsPacket.answers.length > 0 &&
param.responseDecodedDnsPacket.answers[0].type == "CNAME"
Expand Down Expand Up @@ -76,6 +78,7 @@ function checkHttpsSvcbBlock(
}
}
}

function checkCnameBlock(param, response, dnsBlockOperation) {
let cname = param.responseDecodedDnsPacket.answers[0].data.trim().toLowerCase();
let domainNameBlocklistInfo = param.blocklistFilter.getDomainInfo(
Expand Down Expand Up @@ -111,3 +114,10 @@ function checkCnameBlock(param, response, dnsBlockOperation) {
}
}
}

function hasBlocklistStamp(param) {
return param &&
param.userBlocklistInfo &&
!util.emptyString(param.userBlocklistInfo.userBlocklistFlagUint);
}

20 changes: 17 additions & 3 deletions src/helpers/currentRequest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import * as util from "../helpers/util.js";
export default class CurrentRequest {
constructor() {
this.blockedB64Flag = "";
this.decodedDnsPacket = undefined;
this.decodedDnsPacket = this.emptyDecodedDnsPacket();
this.httpResponse = undefined;
this.isException = false;
this.exceptionStack = undefined;
Expand All @@ -25,21 +25,34 @@ export default class CurrentRequest {
this.dnsParser = new DnsParser();
}

emptyDecodedDnsPacket() {
return { id: 0, questions: null };
}

initDecodedDnsPacketIfNeeded() {
if (!this.decodedDnsPacket) {
this.decodedDnsPacket = this.emptyDecodedDnsPacket();
}
}

dnsExceptionResponse() {
this.initDecodedDnsPacketIfNeeded();

const qid = this.decodedDnsPacket.id;
const questions = this.decodedDnsPacket.questions;
const ex = {
exceptionFrom: this.exceptionFrom,
exceptionStack: this.exceptionStack,
};
const servfail = dnsutil.servfail(qid, questions);
this.httpResponse = new Response(
dnsutil.servfail(qid, questions),
{
servfail,
headers : util.concatHeaders(
this.headers(),
this.additionalHeader(JSON.stringify(ex)),
),
status : 200, // rfc8484 section-4.2.1
status : (servfail) ? 200 : 500, // rfc8484 section-4.2.1
},
);
}
Expand Down Expand Up @@ -67,6 +80,7 @@ export default class CurrentRequest {
}

dnsBlockResponse() {
this.initDecodedDnsPacketIfNeeded();
try {
this.decodedDnsPacket.type = "response";
this.decodedDnsPacket.rcode = "NOERROR";
Expand Down
11 changes: 8 additions & 3 deletions src/helpers/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export function corsHeaders() {

/**
* @param {String} ua - User Agent string
* @returns
* @returns
*/
export function corsHeadersIfNeeded(ua) {
// allow cors when user agents claiming to be browsers
Expand Down Expand Up @@ -104,7 +104,7 @@ export function concatHeaders() {

/**
* @param {Request} request - Request
* @returns
* @returns
*/
export function copyHeaders(request) {
const headers = {}
Expand Down Expand Up @@ -213,7 +213,7 @@ export function safeBox(fn, defaultResponse = null) {

/**
* @param {Request} req - Request
* @returns
* @returns
*/
export function isDnsMsg(req) {
return req.headers.get("Accept") === "application/dns-message" ||
Expand All @@ -240,3 +240,8 @@ export function errResponse(id, err) {
data: false,
}
}

export function emptyString(str) {
return !str || str.length === 0

This comment has been minimized.

Sign in to view

Copy link
@amithm7

amithm7 Dec 24, 2021

Contributor

Why is this required? Empty string in JS, i.e. "" will evaluate to false anyway.

This comment has been minimized.

Sign in to view

Copy link
@ignoramous

ignoramous Dec 24, 2021 •
edited

Author Contributor

undefined and null are assigned to variables expected to be strings.

null and undefined do not type coerce to an empty string ("") with conditionals such as == or !=.

This comment has been minimized.

Sign in to view

Copy link
@amithm7

amithm7 Dec 25, 2021 •
edited

Contributor

Yes, but "" , null & undefined are all falsy values.

!"" => true
!null => true
!undefined => true

str.length === 0 will never be executed, when str is "", null or undefined

This comment has been minimized.

Sign in to view

Copy link
@ignoramous

ignoramous Dec 25, 2021

Author Contributor

I'm missing a str.trim().length...
}

0 comments on commit bc6dc50

Please sign in to comment.